Use Amount type for TxOut value field by yancyribbens · Pull Request #1811 · rust-bitcoin/rust-bitcoin

yancyribbens · 2023-04-24T14:49:06Z

Propose using Amount type for the TxOut value field. I only implemented Decodable and Encodable enough to compile but this needs to completed obviously if using Amount seems like a good idea.

yancyribbens · 2023-04-24T14:58:20Z

Also the tests would need to be updated as well to use Amount

RCasatta · 2023-04-24T15:38:00Z

I inadvertently pressed ready for review on mobile and now I can't find how to revert, sorry

Kixunil · 2023-04-24T16:03:23Z

Reverted back to draft.

Concept ACK, I originally wanted to do this myself, happy to see I won't have to! :)

Kixunil · 2023-04-24T16:05:36Z

bitcoin/src/blockdata/transaction.rs

Unrelated but looks like this should be an associated constant TxOut::NULL

Agreed. And we probably shouldn't use default to construct it because it's actually a pretty obscure thing.

Heads up that this is not yet addressed, but I'm going to merge this anyway since it's a pretty big PR that'll be hard to re-review.

I don't think this can be made an associated constant because ScriptBuf::new() is not a const.

impl TxOut { pub const NULL: Self = TxOut { value: Amount::from_sat(0xffffffffffffffff), script_pubkey: ScriptBuf::new() } } } .. }

Is there a reason this needs to be an associated const?

bitcoin/src/psbt/mod.rs

yancyribbens · 2023-04-26T08:47:57Z

hmm almost ready. Looks like there are some changes for nightly and 1.48.0 needed still.

Kixunil · 2023-04-26T11:58:25Z

Looks like it's not 1.48-related but serde-related.

yancyribbens · 2023-04-27T10:02:21Z

I believe the CI failure is unrelated to this PR. Seems like cross rs can't find the docker container currently.

yancyribbens · 2023-04-27T10:02:49Z

Otherwise, this should be good to ship imo

tcharding

ACK 2c4b8e82c45a499e12f774db7623f94ff0214f5d

tcharding · 2023-04-27T23:04:35Z

bitcoin/src/crypto/sighash.rs

Suggested change

/// use bitcoin::Amount;

/// use bitcoin::{absolute, Transaction, Script};

/// use bitcoin::{absolute, Amount, Transaction, Script};

tcharding · 2023-04-27T23:11:03Z

bitcoin/src/amount.rs

consensus_decode is inlined, might as well do this one too.

tcharding · 2023-04-27T23:14:52Z

bitcoin/examples/taproot-psbt.rs

Recently @Kixunil made a push for using a custom integer format for bitcoin values 1_000_000_00, requires

#[allow(clippy::inconsistent_digit_grouping)] // Group to show 100,000,000 sats per bitcoin.

You can see a bunch of these in amount.rs. You could use it if you like it.

I guess we should have Amount::from_whole_btc accepting u64. But also 50 * Amount::ONE_BTC should work.

I tried both 50 * Amount::ONE_BTC and Amount::from_str("50 BTC").unwrap() here but it's not currently possible because this is a const so the compiler doesn't allow it.

We should probably have #![allow(clippy::inconsistent_digit_grouping)] on the whole crate. This lint is stupid and has a 100% false positive rate for me.

Also to avoid const issues we should have constants not only for 1 btc, but also 10, 25, 50, 100 and 21 million. And .1 and .01. Probably also 10, 100, 1000, 10000, 100000 sats (1_000_000 sats is already covered as .01 BTC).

These constants are all 1 LOC (plus a comment plus a whitespace) and will never change, so there's no harm in having a pile of them.

@Kixunil in rust-bech32 we are panicking in a constfn; see discussion here rust-bitcoin/rust-bech32#103 (comment)

I think in 1.48 it's ok?

Panicking is only const since 1.57.

But we could conditionally use it to provide a better error message in higher versions.

Do we have examples of version conditionals already in the code base? It looks like things such as rustversion require an added dependency.

@yancyribbens yep, grep the codebase for rust_v_1_53 to see how we've configured some things that show up only on Rust 1.53 and above.

Added a PR here for this #1870

yancyribbens · 2023-04-29T09:25:39Z

Is there some considerations that need to made since this is a breaking API change? Downstream crates that use u64 for TxOut value should get a deprecation warning?

apoelstra · 2023-04-29T13:56:53Z

@yancyribbens unfortunately AFAICT there is no way to make this a "nice" breaking change. But FWIW I think it will get into the first pre-release as the only major thing, and the fix is nearly a search-and-replace operation, so I think it might be okay.

tcharding

ACK 104d206bbb1fb1d778c956f4d313929b4f1d69cc

tcharding · 2023-05-04T05:48:35Z

We are going to get merge conflicts if #1829 merges ahead of this.

Kixunil · 2023-05-04T07:24:10Z

This will actually simplify tons of code that contain conversions between u64 and Amount.

yancyribbens · 2023-05-04T15:02:19Z

rebased to fix merge conflicts.

tcharding

ACK d57ec01

apoelstra

ACK d57ec01

9f7449b Use from_int_btc function for const context (yancy) f93e679 Add from_int_btc function to Amount (yancy) Pull request description: Followup PR from #1811 Added a `const` associated function `from_int_btc()` for Amount. `panic()` in const context is only available after 1.57+ so a work around is provided. ACKs for top commit: tcharding: ACK 9f7449b apoelstra: ACK 9f7449b Tree-SHA512: 7755234f2e573577d754f0224083cb7acc059e58833790fe344b0d9bad0acd89b0f74054d9efcba2133576222c7e9ab8dc3d81ddc10fbdcd4f83638d697118c4

RCasatta marked this pull request as ready for review April 24, 2023 15:34

Kixunil marked this pull request as draft April 24, 2023 16:02

Kixunil reviewed Apr 24, 2023

View reviewed changes

yancyribbens force-pushed the txout-use-amount-type branch from f987255 to 35eedb9 Compare April 26, 2023 08:36

yancyribbens marked this pull request as ready for review April 26, 2023 08:37

yancyribbens force-pushed the txout-use-amount-type branch from 35eedb9 to c8f2428 Compare April 26, 2023 08:42

yancyribbens changed the title ~~WIP: Use Amount type for TxOut value field~~ Use Amount type for TxOut value field Apr 26, 2023

yancyribbens force-pushed the txout-use-amount-type branch 4 times, most recently from b49da67 to 2c4b8e8 Compare April 27, 2023 09:13

tcharding previously approved these changes Apr 27, 2023

View reviewed changes

yancyribbens dismissed tcharding’s stale review via 104d206 April 28, 2023 08:13

yancyribbens force-pushed the txout-use-amount-type branch from 2c4b8e8 to 104d206 Compare April 28, 2023 08:13

tcharding previously approved these changes May 4, 2023

View reviewed changes

yancyribbens dismissed tcharding’s stale review via d57ec01 May 4, 2023 15:00

yancyribbens force-pushed the txout-use-amount-type branch from 104d206 to d57ec01 Compare May 4, 2023 15:00

Use Amount type for TxOut value field

d57ec01

tcharding approved these changes May 4, 2023

View reviewed changes

apoelstra approved these changes May 4, 2023

View reviewed changes

apoelstra merged commit 25f569a into rust-bitcoin:master May 5, 2023

yancyribbens mentioned this pull request May 5, 2023

Move and rename TxOut default trait to a const called NULL #1838

Merged

yancyribbens mentioned this pull request May 24, 2023

Add from_int_btc method to Amount #1870

Merged

apoelstra mentioned this pull request Nov 28, 2024

units: Remove serde from amounts #3672

Merged

	/// use bitcoin::Amount;
	/// use bitcoin::{absolute, Transaction, Script};
	/// use bitcoin::{absolute, Amount, Transaction, Script};

Conversation

yancyribbens commented Apr 24, 2023

Uh oh!

yancyribbens commented Apr 24, 2023

Uh oh!

RCasatta commented Apr 24, 2023

Uh oh!

Kixunil commented Apr 24, 2023

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!

yancyribbens commented Apr 26, 2023

Uh oh!

Kixunil commented Apr 26, 2023

Uh oh!

yancyribbens commented Apr 27, 2023

Uh oh!

yancyribbens commented Apr 27, 2023

Uh oh!

tcharding left a comment

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!

yancyribbens commented Apr 29, 2023

Uh oh!

apoelstra commented Apr 29, 2023

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented May 4, 2023

Uh oh!

Kixunil commented May 4, 2023

Uh oh!

yancyribbens commented May 4, 2023

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development