Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change Prevouts::All(&[TxOut]) to Prevouts::All(&[&TxOut]) #835

Merged
merged 1 commit into from
Feb 17, 2022
Merged

Change Prevouts::All(&[TxOut]) to Prevouts::All(&[&TxOut]) #835

merged 1 commit into from
Feb 17, 2022

Conversation

sanket1729
Copy link
Member

I believe this avoids some allocation of creating a vec of TxOut to
create a slice incase the data is already available in psbt/other
methods.

See #834

@sanket1729 sanket1729 mentioned this pull request Feb 17, 2022
Closed
@sanket1729 sanket1729 added this to the 0.28.0 milestone Feb 17, 2022
@tcharding
Copy link
Member

tcharding commented Feb 17, 2022 •
edited
Loading

This PR closes #834, right? You used 'see #834', so I was left wondering?

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 17, 2022

I'm not convinced by this. This change helps the caller who already has &[&TxOut] while before it benefited the caller already having &[TxOut]. Either case requires conversion to the other.
From quick look I believe T: Iterator, T::Item: Borrow<TxOut> should work. I suggest to use that unless there's something I don't see.

@sanket1729
Copy link
Member Author

This change helps the caller who already has &[&TxOut] while before it benefited the caller already having &[TxOut].

Yup, exactly I am not sure how to evaluate this. For the psbt workflow, Having [& [&TxOut]] is more useful because the txouts are in separate fields of the pbst data structure. Converting from &[TxOut] to &[&TxOut] seems much better than going the other way.

I can also imagine someone asking a full node gettxout and they have &[TxOut]. I see that this is not strictly better, which is why I raised #834 for more discussion.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 17, 2022

IMO we should try generics with Iterator first and see if it can work.

@sanket1729
Copy link
Member Author

@Kixunil, iterator turned out more annoying(requiring exact size iterators and cloning while consuming) and unneeded for this application. But the borrow::Borrow helped a lot. I believe this satisfies my use-case while retaining the current use-case too.

Kixunil
Kixunil reviewed Feb 17, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, Borrow on item in the array looks good!

src/util/sighash.rs Outdated Show resolved Hide resolved
sanket1729
sanket1729 commented Feb 17, 2022
View reviewed changes
src/util/sighash.rs
@@ -988,7 +997,7 @@ mod tests {
let prevouts = if sighash_type.split_anyonecanpay_flag().1 && tx_bytes[0] % 2 == 0 {
// for anyonecanpay the `Prevouts::All` variant is good anyway, but sometimes we want to
// test other codepaths
Prevouts::One(input_index, &prevouts[input_index])
Prevouts::One(input_index, prevouts[input_index].clone())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clone was done because the generic T needed to be the same type across if else arms

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter much in test anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, just highlighted because it might confuse review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kinda unfortunate because it might be a common pattern, but I don't see any sensible way around it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The taproot_signature_hash function also gets the correct data for sighash single from prevouts::All. I think it is required to extract the corresponding txout from an array in this fashion. This was only done to test that both of these work.

@sanket1729
Change Prevouts::All(&[TxOut]) to Prevouts::All(&[Borrow<T>])
10fedfb 
This avoids some allocation of creating a vec of TxOut to
create a slice incase the data is already available in psbt/other
methods. Facilitates creation of Prevouts from &[TxOut] as well as
&[&TxOut]
apoelstra
apoelstra approved these changes Feb 17, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 10fedfb

TIL about Borrow. Seems like it's a strictly more useful AsRef.

Kixunil
Kixunil approved these changes Feb 17, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 10fedfb

@apoelstra apoelstra merged commit 04787d4 into rust-bitcoin:master Feb 17, 2022
@sanket1729 sanket1729 mentioned this pull request Feb 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sanket1729 @tcharding @Kixunil @apoelstra