Implement PartiallySignedTransaction::fee #1338

hashmap · 2022-10-22T19:01:17Z

to calculate fee if previous outputs are available.
Closes #1220

apoelstra

ACK 67a900a8d5c6e3e6095cfa87f542bb6c37e6c07c

Kixunil

Thanks! Apart from missing overflow handling (which is a critical bug) it looks quite good.

Kixunil · 2022-10-23T15:49:42Z

bitcoin/src/util/psbt/error.rs

@@ -101,6 +103,7 @@ impl fmt::Display for Error {
            },
            Error::CombineInconsistentKeySources(ref s) => { write!(f, "combine conflict: {}", s) },
            Error::ConsensusEncoding => f.write_str("bitcoin consensus or BIP-174 encoding error"),
+            Error::NegativeFee => f.write_str("negative fee is not allowed"),


Hmm I think more context like "the PSBT has a negative fee which is not allowed" would be easier to understand.

Kixunil · 2022-10-23T15:50:46Z

bitcoin/src/util/psbt/mod.rs

+        let mut inputs = 0;
+        for utxo in  self.iter_funding_utxos() {
+            inputs += match utxo {
+                Err(_) => return Err(Error::MissingUtxo),


Why ignore the error and why not use ? ?

Kixunil · 2022-10-23T15:51:42Z

bitcoin/src/util/psbt/mod.rs

+    pub fn fee(&self) -> Result<u64, Error> {
+        let mut inputs = 0;
+        for utxo in  self.iter_funding_utxos() {
+            inputs += match utxo {


Overflow is not handled. I suggest explicitly checking and returning Overflow error.

fixed, added an error variant

Kixunil · 2022-10-23T15:52:07Z

bitcoin/src/util/psbt/mod.rs

+                Ok(utxo) => utxo.value,
+            }
+        }
+        let outputs:u64 = self.unsigned_tx.output.iter().map(|out| out.value).sum();


Again overflow not handled.

Kixunil · 2022-10-23T15:52:43Z

bitcoin/src/util/psbt/mod.rs

+        assert!(t3.fee().is_err());
+
+    }
+


Please add a test for overflow case when you fix it. This is a very important one.

tcharding

Thanks for the PR! My review comments are just suggested improvements to docs, the logic of this PR looks good to me.

ACK cfa75a5109f19c2f02c62f1d9fcb589a91c6453d

tcharding · 2022-10-23T21:15:19Z

bitcoin/src/util/psbt/mod.rs

+    /// Calculates fee
+    ///


Possible docs improvement, suggestion only.

/// Calculates transaction fee. /// /// 'Fee' being the amount that will be paid for mining a transaction with the current inputs /// and outputs i.e., the difference in value of the total inputs and the total outputs.

thanks, updated

tcharding · 2022-10-23T21:20:52Z

bitcoin/src/util/psbt/mod.rs

+    /// The function returns [`Error::MissingUtxo`] when UTXO information for at least one input
+    /// is not present or is invalid.
+    /// If calculated value is negative [`Error::NegativeFee`] is returned.
+    /// If an integer overflow happens [`Error::FeeOverflow`] is returned.


This will be rendered in HTML as a single sentence, perhaps either put it as a single sentence in code

/// The function returns [`Error::MissingUtxo`] when UTXO information for at least one input is /// not present or is invalid. If calculated value is negative [`Error::NegativeFee`] is /// returned. If an integer overflow happens [`Error::FeeOverflow`] is returned.

or as a list if that's preferred

/// - [`Error::MissingUtxo`] when UTXO information for any input is not present or is invalid. /// - [`Error::NegativeFee`] if calculated value is negative. /// - [`Error::FeeOverflow`] if an integer overflow occurs.

In case its useful for you, here is a shell command to build the docs using our custom conditional configuration option docsrs:
RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --all-features

appreciate your detailed suggestion, updated

tcharding · 2022-10-23T21:28:06Z

Your CI fails are:

Remove clone when setting t4 in tests
Use max_value() instead of MAX because MAX is not available in Rust 1.41.1 (our current MSRV)

Kixunil

ACK cfa75a5109f19c2f02c62f1d9fcb589a91c6453d

Thank you!

hashmap · 2022-10-24T08:53:49Z

Your CI fails are:

* Remove `clone` when setting `t4` in tests

* Use `max_value()` instead of `MAX` because `MAX` is not available in Rust 1.41.1 (our current MSRV)

Thanks, fixed. Unfortunately aarch64-apple-darwin binaries are available starting from Rust 1.49, will see how to get around it.

Kixunil · 2022-10-24T09:58:32Z

Unfortunately aarch64-apple-darwin binaries are available starting from Rust 1.49

I assume you're saying you use M1, I think x68_64 binaries should still work even though they would be slightly less efficient.

Kixunil · 2022-10-24T10:01:12Z

bitcoin/src/util/psbt/mod.rs

+    /// - [`Error::MissingUtxo`] when UTXO information for any input is not present or is invalid.
+    /// - [`Error::NegativeFee`] if calculated value is negative.
+    /// - [`Error::FeeOverflow`] if an integer overflow occurs.
+    pub fn fee(&self) -> Result<u64, Error> {


Oh, wait, sorry for being late I only realized now. We should return Result<Amount, Error> here.

done, I left calculations using u64 because psbt values are u64 (why btw?)

Yep, that's correct.

why

An oversight in API design. We have an issue to change it but nobody did it yet. (We have a bunch of APIs we want to improve so if you're surprised next time it's probably the same reason.)

hashmap · 2022-10-24T12:27:07Z

@Kixunil sorry, I should have been be more specific. On M1 I see

% RUSTUP_TOOLCHAIN=1.41.1 ./contrib/test.sh
+ CRATES=bitcoin
+ for crate in '${CRATES}'
+ cd bitcoin
+ ./contrib/test.sh
+ FEATURES='base64 bitcoinconsensus serde rand secp-recovery'
+ '[' '' = true ']'
+ cargo --version
info: syncing channel updates for '1.41.1-aarch64-apple-darwin'
info: latest update on 2020-02-27, rust version 1.41.1 (f3e1a954d 2020-02-24)
error: target 'aarch64-apple-darwin' not found in channel.  Perhaps check https://doc.rust-lang.org/nightly/rustc/platform-support.html for available targets

I'll try to fix it later on.

to calculate fee if previous outputs are available.

Kixunil · 2022-10-24T15:39:15Z

I think this could work: rustup toolchain install 1.41.1-x86_64-apple-darwin
You probably need to run it with cargo +1.41.1 test --target x86_64-apple-darwin though, not sure.

Kixunil

ACK c34d5f8 if CI passes

tcharding

ACK c34d5f8

apoelstra

ACK c34d5f8

hashmap force-pushed the issue-1220-psbt-fee branch 2 times, most recently from 9514408 to 67a900a Compare October 22, 2022 19:20

apoelstra previously approved these changes Oct 23, 2022

View reviewed changes

Kixunil requested changes Oct 23, 2022

View reviewed changes

hashmap dismissed apoelstra’s stale review via aa3b620 October 23, 2022 16:23

hashmap force-pushed the issue-1220-psbt-fee branch 3 times, most recently from bb0979e to cfa75a5 Compare October 23, 2022 17:59

tcharding previously approved these changes Oct 23, 2022

View reviewed changes

Kixunil previously approved these changes Oct 23, 2022

View reviewed changes

hashmap dismissed stale reviews from Kixunil and tcharding via 06c5e2a October 24, 2022 08:49

hashmap force-pushed the issue-1220-psbt-fee branch from cfa75a5 to 06c5e2a Compare October 24, 2022 08:49

Kixunil reviewed Oct 24, 2022

View reviewed changes

Kixunil mentioned this pull request Oct 24, 2022

units: Improve Amount type with guarantees on value being in range #620

Closed

Implement PartiallySignedTransaction::fee

c34d5f8

to calculate fee if previous outputs are available.

hashmap force-pushed the issue-1220-psbt-fee branch from 06c5e2a to c34d5f8 Compare October 24, 2022 12:40

Kixunil approved these changes Oct 24, 2022

View reviewed changes

tcharding approved these changes Oct 24, 2022

View reviewed changes

apoelstra approved these changes Oct 24, 2022

View reviewed changes

apoelstra merged commit 29d3bc0 into rust-bitcoin:master Oct 24, 2022

tcharding added the release notes mention label Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement PartiallySignedTransaction::fee #1338

Implement PartiallySignedTransaction::fee #1338

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Implement PartiallySignedTransaction::fee #1338

Implement PartiallySignedTransaction::fee #1338

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!