Skip to content

Conversation

ulrichard
Copy link
Collaborator

@ulrichard ulrichard commented Jan 15, 2025

I adapted the signature counts in the tests to make the tests pass.
In the old version there was a flag remove_partial_sigs in SignOptions.
I removed the test tampered_proof_sighash_tx because the expected error was not thrown, and apparently doesn't exist any more.

@ulrichard ulrichard force-pushed the feature/v1 branch 3 times, most recently from ea66d36 to 443a165 Compare January 16, 2025 10:35
@ulrichard ulrichard force-pushed the feature/v1 branch 2 times, most recently from 3c3ebfc to 10c2b54 Compare January 23, 2025 10:29
Copy link

@tvpeter tvpeter left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. Waiting for this PR to update reserves feature on bdk-cli. Thank you.

let expected = r#"cHNidP8BAH4BAAAAAmw1RvG4UzfnSafpx62EPTyha6VslP0Er7n3TxjEpeBeAAAAAAD/////2johM0znoXIXT1lg+ySrvGrtq1IGXPJzpfi/emkV9iIAAAAAAP////8BUMMAAAAAAAAZdqkUn3/QltN+0sDj9/DPySS+70/862iIrAAAAAAAAQEKAAAAAAAAAAABUQEHAAABAR9QwwAAAAAAABYAFOzlJlcQU9qGRUyeBmd56vnRUC5qIgYDKwVYB4vsOGlKhJM9ZZMD4lddrn6RaFkRRUEVv9ZEh+ME7OUmVwAA"#;
let expected = r#"cHNidP8BAH4BAAAAAmw1RvG4UzfnSafpx62EPTyha6VslP0Er7n3TxjEpeBeAAAAAAD/////MQvsP2eDTCk3vWfQJ50IOFWLwuTHPsnYikR1hosdK0sAAAAAAP3///8BUMMAAAAAAAAZdqkUn3/QltN+0sDj9/DPySS+70/862iIrAAAAAAAAQEKAAAAAAAAAAABUQEHAAABAR9QwwAAAAAAABYAFOzlJlcQU9qGRUyeBmd56vnRUC5qIgYDKwVYB4vsOGlKhJM9ZZMD4lddrn6RaFkRRUEVv9ZEh+ME7OUmVwAA"#;

assert_eq!(psbt_b64, expected);
Copy link

Choose a reason for hiding this comment

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

This is failing because the Psbt produced is version 2 while the expected Psbt value you supplied is version 1. This is coming from here. Every other data is the same.
Screenshot 2025-02-10 at 5 53 11 PM
Screenshot 2025-02-10 at 5 53 27 PM

@notmandatory
Copy link
Member

You should start with bdk_wallet 1.2, it has some good improvements. https://github.com/bitcoindevkit/bdk/releases/tag/wallet-1.2.0

Otherwise ACK

@ulrichard ulrichard force-pushed the feature/v1 branch 6 times, most recently from 7c55933 to e4b104d Compare April 25, 2025 14:01
@tvpeter
Copy link

tvpeter commented Apr 26, 2025

tACK e4b104d

@notmandatory
Copy link
Member

Looks like you need to take the WASM CI job out for this PR too or you can rebase it after merging #38 is merged.

@ulrichard
Copy link
Collaborator Author

@notmandatory I rebased on master after merging the other branch. So, everything should be fine.

Copy link

@aagbotemi aagbotemi left a comment

Choose a reason for hiding this comment

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

Great job on this pr @ulrichard
Ran all the tests and they pass.
reserves

pub trait ProofOfReserves {
/// Create a proof for all spendable UTXOs in a wallet
fn create_proof(&self, message: &str) -> Result<PSBT, ProofError>;
fn create_proof(&mut self, message: &str) -> Result<Psbt, ProofError>;

Choose a reason for hiding this comment

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

Running cargo clippy --all-features --all-targets -- -D warnings on newer Clippy versions produces a result_large_err error:

error: the `Err`-variant returned from this function is very large
  --> src/reserves.rs:89
   |
89 |     TxExtraction(ExtractTxError),
   |     ---------------------------- the largest variant contains at least 192 bytes

This causes performance issues as every Result<T, ProofError> reserves 192+ bytes on the stack, even for successful operations. The error doesn't appear in CI, likely due to using an older Clippy version.

Proposed Solution: Box the large variant:

TxExtraction(Box<ExtractTxError>),

max_block_height: Option<u32>,
) -> Result<u64, ProofError>;
max_block_height: Option<usize>,
) -> Result<Amount, ProofError>;

Choose a reason for hiding this comment

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

Running cargo clippy --all-features --all-targets -- -D warnings on newer Clippy versions produces result_large_err error here likewise

outpoints: Vec<(OutPoint, TxOut)>,
) -> Result<u64, ProofError> {
let tx = psbt.clone().extract_tx();
) -> Result<Amount, ProofError> {

Choose a reason for hiding this comment

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

Running cargo clippy --all-features --all-targets -- -D warnings on newer Clippy versions produces result_large_err error here likewise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants