Add support for pay to anchor outputs by ErikDeSmedt · Pull Request #4111 · rust-bitcoin/rust-bitcoin

ErikDeSmedt · 2025-02-24T12:21:21Z

Add support for the newly created Pay2Anchor output-type which was introduced in bitcoin 28.0

coveralls · 2025-02-24T12:25:59Z

Pull Request Test Coverage Report for Build 13561507629

Details

21 of 25 (84.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.009%) to 82.734%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
bitcoin/src/address/mod.rs	16	17	94.12%
bitcoin/src/blockdata/script/witness_program.rs	3	6	50.0%

Totals
Change from base Build 13554993820:	0.009%
Covered Lines:	21218
Relevant Lines:	25646

💛 - Coveralls

Kixunil

Concept ACK, the code looks good too except for one issue.

Kixunil · 2025-02-24T13:53:14Z

+    // In SegWit v0, the program must be either 2, 20 or 32 bytes long
+    debug_assert!(
+        version != WitnessVersion::V0 ||
+        program.len() == 2 || program.len() == 20 || program.len() == 32);


I believe this is incorrect. P2A is WitnessVersion::V1.

Correct, I think we need a separate assertion for Segwit v1 witness programs ( P2TR, P2A)
Something like this:

// In SegWit v0, the program must be either 20 or 32 bytes long (P2WPKH/P2WSH) debug_assert!(version != WitnessVersion::V0 || program.len() == 20 || program.len() == 32); // In SegWit v1, the program must be either 2 bytes (P2A) or 32 bytes (P2TR) debug_assert!(version != WitnessVersion::V1 || program.len() == 2 || program.len() == 32);

Yes, that makes sense.

Thanks, this is a great catch. I've adapted the PR

Kixunil

ACK 34cce11

If you're in the mood you could also check if this can be backported backwards-compatibly. I have an application in mind that could use it.

Kixunil · 2025-02-25T12:34:40Z

    }

+    /// Constructs a new pay to anchor address
+    pub fn p2a() -> Self {


I think it might be possible to make it const but that might require some modifications to ArrayVec, so I'm not requiring it. Similarly, we could have a const on Script, but that's just extra, so not required.

This was easier as expected. Apprantly ArrayVec::from_slice is already const.

Kixunil · 2025-02-25T12:37:28Z

+        // Verify that the address is considered standard
+        // and that the output type is P2a
+        assert!(address.is_spend_standard());
+        assert_eq!(address.address_type(), Some(AddressType::P2a));


Testing address.to_string() would be nice too.

I've added a comparision for address.to_string()

Kixunil

ACK 83d7ebb

tcharding

ACK 83d7ebb

tcharding · 2025-02-25T23:34:44Z

Thanks for the contribution!

I created a follow up issue to this PR: #4124. I did so intentionally instead of asking for changes so as not to slow down this PR. We appreciate the effort you went to. You are most welcome to attack the follow up but do not feel obliged.

Thanks again!

Kixunil

Some thoughts crossed my mind and I did some more digging. Unfortunately, this is not right.

Kixunil · 2025-02-26T09:48:59Z

 pub const MAX_SIZE: usize = 40;

+/// The P2A program which is given by 0x4e73
+pub const P2A_PROGRAM: [u8;2] = [78, 115];


Oh, I have now noticed this is public. I don't think it's very helpful and adds noise to docs. I'd prefer to keep it private until there's a use case.

I use the P2A_PROGRAM in another module. I have modified to pub(crate)

Kixunil · 2025-02-26T10:24:37Z

+    // In SegWit v0, the program must be either 20 (P2WPKH) bytes or 32 (P2WSH) bytes long
    debug_assert!(version != WitnessVersion::V0 || program.len() == 20 || program.len() == 32);
+    // In SegWit v1, the program must be either 2 bytes (P2A) or 32 bytes (P2TR)
+    debug_assert!(version != WitnessVersion::V1 || program.len() == 2 || program.len() == 32);


I've just realized this entire check is bogus and should be deleted. For witness versions > 0 receiving is standard to ensure future compatibility but this check (even if in debug) undermines that compatibility. It's also the reason it wasn't here in the first place. (And perhaps a reason to write a comment "other versions intentionally not checked" to avoid this mistake again.) The check only makes sense for v0 because that one makes different lengths unspendable.

I have removed the check

apoelstra

ACK 83d7ebb; successfully ran local tests

Add support for the newly created Pay2Anchor output-type. See bitcoin/bitcoin#30352

Kixunil

ACK f7ea6e5

apoelstra

ACK f7ea6e5; successfully ran local tests

Manually backport PR rust-bitcoin#4111. Of note, here we put `new_p2a` on `ScriptBuf` instead of on the `script::Builder` because that seems to be where all the other `new_foo` methods are in this release. From the original patch: Add support for the newly created Pay2Anchor output-type. See bitcoin/bitcoin#30352

Manually backport PR rust-bitcoin#4111. Of note, here we put `new_p2a` on `ScriptBuf` instead of on the `script::Builder` because that seems to be where all the other `new_foo` methods are in this release. Note the `WitnessProgram::p2a` is conditionally const on Rust `v1.61` because MSRV is only `v1.56.1`. From the original patch: Add support for the newly created Pay2Anchor output-type. See bitcoin/bitcoin#30352

c2481e4 backport: Add support for pay to anchor outputs (Tobin C. Harding) Pull request description: Manually backport PR #4111. Of note, here we put `new_p2a` on `ScriptBuf` instead of on the `script::Builder` because that seems to be where all the other `new_foo` methods are in this release. From the original patch: Add support for the newly created Pay2Anchor output-type. See bitcoin/bitcoin#30352 ACKs for top commit: apoelstra: ACK c2481e4; successfully ran local tests Tree-SHA512: 016919914750adf6f8226acb4e6b36c0dcd8ce230df8cca13f19bcc97709caf07a076be367c76f9519a421fc93fdf73caa078ee65a85a750af7a9d9e6c757e75

Manually backport PR rust-bitcoin#4111. Of note, here we put `new_p2a` on `ScriptBuf` instead of on the `script::Builder` because that seems to be where all the other `new_foo` methods are in this release. Note the `WitnessProgram::p2a` is conditionally const on Rust `v1.61` because MSRV is only `v1.56.1`. From the original patch: Add support for the newly created Pay2Anchor output-type. See bitcoin/bitcoin#30352

github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Feb 24, 2025

ErikDeSmedt force-pushed the erikdesmedt/p2a branch from a4f2d09 to 9c351f2 Compare February 24, 2025 12:24

ErikDeSmedt force-pushed the erikdesmedt/p2a branch from 9c351f2 to e7c49b9 Compare February 24, 2025 12:28

Kixunil reviewed Feb 24, 2025

View reviewed changes

ErikDeSmedt force-pushed the erikdesmedt/p2a branch from e7c49b9 to 34cce11 Compare February 25, 2025 11:09

Kixunil previously approved these changes Feb 25, 2025

View reviewed changes

ErikDeSmedt dismissed Kixunil’s stale review via ab0eaeb February 25, 2025 13:05

ErikDeSmedt force-pushed the erikdesmedt/p2a branch 2 times, most recently from ab0eaeb to 83d7ebb Compare February 25, 2025 13:09

Kixunil approved these changes Feb 25, 2025

View reviewed changes

tcharding previously approved these changes Feb 25, 2025

View reviewed changes

tcharding mentioned this pull request Feb 25, 2025

A few minor witness program issues #4124

Closed

Kixunil requested changes Feb 26, 2025

View reviewed changes

apoelstra previously approved these changes Feb 26, 2025

View reviewed changes

Add support for pay to anchor outputs

f7ea6e5

Add support for the newly created Pay2Anchor output-type. See bitcoin/bitcoin#30352

ErikDeSmedt dismissed stale reviews from apoelstra and tcharding via f7ea6e5 February 27, 2025 07:57

ErikDeSmedt force-pushed the erikdesmedt/p2a branch from 83d7ebb to f7ea6e5 Compare February 27, 2025 07:57

Kixunil approved these changes Feb 27, 2025

View reviewed changes

apoelstra approved these changes Feb 28, 2025

View reviewed changes

apoelstra merged commit 6518125 into rust-bitcoin:master Feb 28, 2025

storopoli mentioned this pull request Mar 7, 2025

Add Pay To Anchor (P2A) output support alpenlabs/bitcoin-bosd#55

Closed

This was referenced Apr 6, 2025

Socratic Seminar #30 Topics NashBitDevs/nashbitdevs.org#82

Closed

DC BitDevs 014 Topic Suggestion dcbitdevs/dcbitdevs.github.io#24

Closed

tcharding mentioned this pull request May 5, 2025

Tag a new release 0.32.6 #4444

Closed

This was referenced May 14, 2025

chore(deps): bump bitcoin to 0.32.6 alpenlabs/alpen#804

Merged

chore(deps): bump bitcoin to 0.32.6 alpenlabs/strata-bridge#144

Merged

tcharding mentioned this pull request Jul 8, 2025

backport: Add support for pay to anchor outputs #4691

Merged

Conversation

ErikDeSmedt commented Feb 24, 2025

Uh oh!

coveralls commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 13561507629

Details

💛 - Coveralls

Uh oh!

Kixunil 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!

Kixunil 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!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Feb 25, 2025

Uh oh!

Kixunil 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!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil 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

Uh oh!

6 participants

coveralls commented Feb 24, 2025 •

edited

Loading