policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending

instagibbs commented at 7:32 pm on June 27, 2024: member

This is a sub-feature taken out of the original proposal for ephemeral anchors #30239

This PR makes spending of OP_1 <0x4e73> (i.e. bc1pfeessrawgf) standard. Creation of this output type is already standard.

Any future witness output types are considered relay-standard to create, but not to spend. This preserves upgrade hooks, such as a completely new output type for a softfork such as BIP341. It also gives us a bit of room to use a new output type for policy uses.

This particular sized witness program has no other known use-cases (https://bitcoin.stackexchange.com/a/110664/17078), s it affords insufficient cryptographic security for a secure commitment to data, such as a script or a public key. This makes this type of output “keyless”, or unauthenticated.

As a witness program, the scriptSig of the input MUST be blank, by BIP141. This helps ensure txid-stability of the spending transaction, which may be required for smart contracting wallets. If we do not use segwit, a miner can simply insert an OP_NOP in the scriptSig without effecting the result of program execution.

An additional relay restriction is to disallow non-empty witness data, which an adversary may use to penalize the “honest” transactor when RBF’ing the transaction due to the incremental fee requirement of RBF rules.

The intended use-case for this output type is to “anchor” the transaction with a spending child to bring exogenous CPFP fees into the transaction package, encouraging the inclusion of the package in a block. The minimal size of creation and spending of this output makes it an attractive contrast to outputs like p2sh(OP_TRUE) and p2wsh(OP_TRUE) which are significantly larger in vbyte terms.

Combined with TRUC transactions which limits the size of child transactions significantly, this is an attractive option for presigned transactions that need to be fee-bumped after the fact.

DrahtBot commented at 7:32 pm on June 27, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, sdaftuar, ismaelsadeeq, glozow, tdb3
Concept NACK	luke-jr
Concept ACK	petertodd, achow101
Stale ACK	t-bast

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28241 (Silent payment index (for light wallets and consistency check) by Sjors)
#28201 (Silent Payments: sending by josibake)
#28122 (Silent Payments: Implement BIP352 by josibake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label TX fees and policy on Jun 27, 2024

bitcoin deleted a comment on Jun 28, 2024

Christewart approved

Christewart commented at 8:02 pm on June 29, 2024: contributor

utack 99e9d3c7bc45e1266c6592b2afd47b8999745e04

instagibbs renamed this:
~~policy: Add `OP_TRUE <0x4e73>` as a standard output script~~
policy: Add `OP_TRUE <0x4e73>` as a standard output script for spending
on Jul 2, 2024

instagibbs renamed this:
~~policy: Add `OP_TRUE <0x4e73>` as a standard output script for spending~~
policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending
on Jul 2, 2024

theStack commented at 4:33 pm on July 3, 2024: contributor

Suggestion for adding corresponding decodescript test coverage:

 0diff --git a/test/functional/rpc_decodescript.py b/test/functional/rpc_decodescript.py
 1index f37e61ab50..a105b3d3b4 100755
 2--- a/test/functional/rpc_decodescript.py
 3+++ b/test/functional/rpc_decodescript.py
 4@@ -187,6 +187,16 @@ class DecodeScriptTest(BitcoinTestFramework):
 5         assert_equal('1 ' + xonly_public_key, rpc_result['asm'])
 6         assert 'segwit' not in rpc_result
 7 
 8+        self.log.info("- P2A (anchor)")
 9+        # 1 <4e73>
10+        witprog_hex = '4e73'
11+        rpc_result = self.nodes[0].decodescript('5102' + witprog_hex)
12+        assert_equal('anchor', rpc_result['type'])
13+        # in the disassembly, the witness program is shown as single decimal due to its small size
14+        witprog_as_decimal = int.from_bytes(bytes.fromhex(witprog_hex), 'little')
15+        assert_equal(f'1 {witprog_as_decimal}', rpc_result['asm'])
16+        assert 'fees' in rpc_result['address']
17+
18     def decoderawtransaction_asm_sighashtype(self):
19         """Test decoding scripts via RPC command "decoderawtransaction".

I assume that the witness program was chosen on purpose to contain “fees” in the bech32m encoded address and this was not just by accident? (could also test for the full address anyways)

luke-jr commented at 5:04 pm on July 6, 2024: member

Concept NACK. This seems to encourage and promote spam. We should be doing the exact opposite.

Curious why you say “creation was already standard” - I don’t see why that would be the case.

petertodd commented at 0:55 am on July 7, 2024: contributor

Concept NACK. This seems to encourage and promote spam. We should be doing the exact opposite.

This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed. The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It has nothing to do with “spam”.

Curious why you say “creation was already standard” - I don’t see why that would be the case.

Because we decided to make wallets and Bitcoin Core itself forward compatible with future tapscript versions. If this class of outputs was not standard, you’d have a big transition time before a new type of tapscript was actually usable for general purpose wallets.

Concept ACK.

in src/script/interpreter.cpp:1946 in 5e7e767bc7 outdated

1942@@ -1943,6 +1943,9 @@ static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion,
1943             }
1944             return set_success(serror);
1945         }
1946+    } else if (witversion == 1 && program.size() == 2 && program[0] == 0x4e && program[1] == 0x73) {

maflcko commented at 11:11 am on July 8, 2024:

5e7e767bc79682de3879b8546294d50542570092: Intentional to make p2sh-p2a spend standard as well? Would be good to have a test as documentation, so that reviewers know what to expect and don’t have to guess.

instagibbs commented at 6:28 pm on July 8, 2024:

this was unintentional. Disallowed it and added coverage.

There is no situation where this would be preferable.

instagibbs force-pushed on Jul 8, 2024

instagibbs commented at 6:30 pm on July 8, 2024: member

@theStack took test coverage suggestion, and yes “fees” is intentional since it will be in any net’s address @luke-jr I think @petertodd has stated what I would say. It’s a pattern people can already do, and it’s trivial and cheap to clean up.

instagibbs force-pushed on Jul 8, 2024

DrahtBot added the label CI failed on Jul 8, 2024

DrahtBot commented at 6:31 pm on July 8, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/27179493102

luke-jr commented at 6:42 pm on July 8, 2024: member

This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.

And at a reduced cost. Unless this proposal increases vsize to make it identical?

The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It has nothing to do with “spam”.

What is the 0x4e73 for?

These outputs bloat the UTXO set, without sufficient value to incentivise cleanup later. The anchor idea is documented to address this by requiring they be spent before being accepted, but that isn’t the case in this PR (unless I missed something).

Curious why you say “creation was already standard” - I don’t see why that would be the case.

Because we decided to make wallets and Bitcoin Core itself forward compatible with future tapscript versions.

I didn’t notice this was a taproot output. In that case, my question is why that is the case. Shouldn’t an output with just OP_TRUE in scriptPubKey suffice for this use case?

petertodd commented at 7:07 pm on July 8, 2024: contributor

On July 8, 2024 8:42:50 PM GMT+02:00, Luke Dashjr @.***> wrote:

This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.

And at a reduced cost. Unless this proposal increases vsize to make it identical?

The cost reduction is a natural one, in line with the byte reduction.

The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It has nothing to do with “spam”.

What is the 0x4e73 for?

Good question! Maybe that’s not needed.

These outputs bloat the UTXO set, without sufficient value to incentivise cleanup later. The anchor idea is documented to address this by requiring they be spent before being accepted, but that isn’t the case in this PR (unless I missed something).

They bloat the UTXO set no more than the alternative of keyed anchor outputs, which are already used by Lightning. Slightly less actually, as keyless anchor outputs are smaller.

The lower dust threshold for them is in line with the decrease in # of bytes necessary to spend them.

Curious why you say “creation was already standard” - I don’t see why that would be the case.

Because we decided to make wallets and Bitcoin Core itself forward compatible with future tapscript versions.

I didn’t notice this was a taproot output. In that case, my question is why that is the case. Shouldn’t an output with just OP_TRUE in scriptPubKey suffice for this use case?

A bare OP_True scriptPubKey leads to a malleable transaction because it isn’t segwit. That’s a serious problem for certain use cases.

IMO it would be reasonable to also allow a bare OP_True scriptPubKey for the usecases where that malleability isn’t important. But it’s reasonable to do both (and actually, I think we can do this with an entirely empty scriptPubKey, which is only (standardly) spendable via OP_True).

instagibbs commented at 7:56 pm on July 8, 2024: member

What is the 0x4e73 for?

To be segwit, it needs to be a witness version + witness program(or p2sh nested segwit script). A witness program is 2 to 40 bytes long push. For this use-case we simply use size two. The specific bytes chosen are simply “cutesy” to spell “fees”, as they cannot be 0 so something else has to be chosen and have an associated address. As far as I know this is the only proposed size 2 witness program use case so far.

These outputs bloat the UTXO set, without sufficient value to incentivise cleanup later. The anchor idea is documented to address this by requiring they be spent before being accepted, but that isn’t the case in this PR (unless I missed something).

If we want sub-dust(as defined per IsDust) the requirement would be this would be “ephemeral dust” #30239 . These are now conceptually distinct features. This PR simply uses the current dust calculations over all witness program types, which actually still penalizes it for assuming it has a P2WSH-like witness when it actually has no witness related data. Further penalizing can happen for further complication, but I’m unconvinced that this would be a net gain.

IMO it would be reasonable to also allow a bare OP_True scriptPubKey for the usecases where that malleability isn’t important. But it’s reasonable to do both (and actually, I think we can do this with an entirely empty scriptPubKey, which is only (standardly) spendable via OP_True).

Indeed, bare OP_TRUE was my original proposal and could still be rolled out at any point. I think paying the 3 vbytes extra to get txid stability is a fair trade for the time being. Lack of txid stability is awful for reasoning about composability :(

instagibbs commented at 9:26 pm on July 8, 2024: member

@achow101 wallet_backwards_compatibility.py --legacy failure seems spurious? can you check?

in test/functional/mempool_accept.py:430 in ee0f688f4c outdated

425+        nested_true_tx.rehash()
426+        self.generateblock(node, self.wallet.get_address(), [nested_true_tx.serialize().hex()])
427+
428+        nested_true_tx_spend = CTransaction()
429+        nested_true_tx_spend.vin.append(CTxIn(COutPoint(nested_true_tx.sha256, 0), b""))
430+        nested_true_tx_spend.vin[0].scriptSig = len(PAY_TO_ANCHOR).to_bytes(1, 'little') + PAY_TO_ANCHOR

maflcko commented at 7:14 am on July 9, 2024:

nit: May be cleaner to defer to script serialization instead of doing it manually?

0        nested_true_tx_spend.vin[0].scriptSig = CScript([bytes(PAY_TO_ANCHOR)])

instagibbs commented at 5:01 pm on July 9, 2024:

thanks, taken

maflcko commented at 7:19 am on July 9, 2024: member

See #29806 for the wallet intermittent unrelated test issue.

instagibbs force-pushed on Jul 9, 2024

Christewart commented at 6:11 pm on July 9, 2024: contributor

IIUC these txs will not get relayed widely across the network until there is widespread adoption of bitcoind versions that adopt these txs as standard?

instagibbs commented at 6:13 pm on July 9, 2024: member

@Christewart spends of these outputs will not propagate until some 10%+ of the network updates, correct.

Christewart commented at 6:14 pm on July 9, 2024: contributor

@Christewart spends of these outputs will not propagate until some 10%+ of the network updates, correct.

Worth mentioning this in the release notes as this feature is intended for time sensitive protocols. Perhaps putting the cart before the horse, but it is a potential foot gun.

instagibbs commented at 6:32 pm on July 9, 2024: member

@Christewart done, since it would need a release note anyways

DrahtBot removed the label CI failed on Jul 9, 2024

instagibbs commented at 1:37 pm on July 15, 2024: member

was chatting offline and was reminded of why I landed on a witness program over doing script trickery like:

OP_DEPTH OP_NOT

the issue here is that a miner could insert a OP_NOP onto the scriptSig, resulting in a different valid txid, breaking presigned transaction chains. Seems to suggest you really do need segwit here.

in src/script/script.cpp:211 in 7013da2227 outdated

203@@ -204,6 +204,15 @@ unsigned int CScript::GetSigOpCount(const CScript& scriptSig) const
204     return subscript.GetSigOpCount(true);
205 }
206 
207+bool CScript::IsPayToAnchor() const
208+{
209+    return (this->size() == 4 &&
210+        (*this)[0] == OP_TRUE &&
211+        (*this)[1] == 0x02 &&

ismaelsadeeq commented at 3:18 pm on July 16, 2024:

whats the 0x02? P2Anchor is OP_TRUE <0x4e73>

instagibbs commented at 3:46 pm on July 16, 2024:

It’s the instruction to push next two bytes on the stack

in src/script/solver.h:25 in 7013da2227 outdated

21@@ -22,6 +22,7 @@ template <typename C> class Span;
22 enum class TxoutType {
23     NONSTANDARD,
24     // 'standard' transaction types:
25+    ANCHOR, //!< anyonecanspendable script

ismaelsadeeq commented at 3:21 pm on July 16, 2024:

0    ANCHOR, //!< anyone can spend script

instagibbs commented at 5:16 pm on July 16, 2024:

done

in src/addresstype.cpp:100 in 7013da2227 outdated

93         addressRet = WitnessUnknown{vSolutions[0][0], vSolutions[1]};
94         return true;
95-    }
96     case TxoutType::MULTISIG:
97     case TxoutType::NULL_DATA:
98     case TxoutType::NONSTANDARD:

ismaelsadeeq commented at 3:27 pm on July 16, 2024:

In 7013da222720fde5ea1c76bba99b1c91ae18558f “policy: Add OP_TRUE <0x4e73> as a standard output type”

Add ANCHOR txout script type to CTxDestination.

instagibbs commented at 5:03 pm on July 16, 2024:

I’m actually having trouble figuring out where this stuff is exposed since we wouldn’t be issuing these addresses as a wallet, and we don’t return “script” unless it’s a p2(w)sh via getaddressinfo. Mind if I punt on this?

instagibbs commented at 3:19 am on July 23, 2024:

ended up adding it since its required if we aren’t returning the “solutions” vector

in src/policy/policy.cpp:228 in 5e286162df outdated

224@@ -225,6 +225,11 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
225         // get the scriptPubKey corresponding to this input:
226         CScript prevScript = prev.scriptPubKey;
227 
228+        // witness stuffing detected

ismaelsadeeq commented at 3:31 pm on July 16, 2024:

~A test for this will be nice!~ I think it’s covered here 1ecc081c601694c351ac90b4698754f1c01530e1

instagibbs commented at 3:47 pm on July 16, 2024:

should have coverage, please check it fails :)

ismaelsadeeq commented at 3:32 pm on July 16, 2024: member

Concept ACK

instagibbs force-pushed on Jul 16, 2024

glozow commented at 2:10 pm on July 18, 2024: member

concept ACK

t-bast approved

t-bast commented at 9:27 am on July 19, 2024: contributor

ACK https://github.com/bitcoin/bitcoin/pull/30352/commits/8d956ccb86d70e2059a5e2ccd8e63b7f7c8e3842, thanks for pushing this!

I’ve read through the code and comments on this PR, and to me it makes sense to use a taproot 2-byte program, the cute trick of having it encode as “fees” in bech32m can be useful (since we need to arbitrarily choose 2 bytes anyway for the program), and it’s a step towards ephemeral anchors which are much better than current anchor outputs :+1:

Regarding utxo bloat when used in lightning, this is also a step in the right direction: it’s better than the current anchor outputs (especially when using taproot channels where it’s hard to reveal the spending key) and once ephemeral anchors are available, we won’t bloat the utxo set at all anymore since we’ll always have a single anchor that will have to be spent to add fees to the 0-fee commitment transaction.

DrahtBot requested review from glozow on Jul 19, 2024

DrahtBot requested review from ismaelsadeeq on Jul 19, 2024

petertodd commented at 3:20 pm on July 19, 2024: contributor

since we’ll always have a single anchor

FYI anchor channels having two anchor outputs is actually a design oversight. Only one anchor output is needed in almost all cases: https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-December/004246.html

t-bast commented at 3:43 pm on July 19, 2024: contributor

FYI anchor channels having two anchor outputs is actually a design oversight. Only one anchor output is needed in almost all cases: https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-December/004246.html

Right, we discussed that in this thread, but the responses to that thread show that it’s not a design oversight, using a single anchor has drawbacks as well. But it’s quite off-topic for this PR though.

petertodd commented at 4:13 pm on July 19, 2024: contributor

I responded to all those points, as you can see in the thread. Anyway, it’s clearly an on topic FYI as it impacts how useful this feature actually is. But no need to discuss it further than the FYI link.

On July 19, 2024 5:43:53 PM GMT+02:00, Bastien Teinturier @.***> wrote:

FYI anchor channels having two anchor outputs is actually a design oversight. Only one anchor output is needed in almost all cases: https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-December/004246.html

Right, we discussed that in this thread, but the responses to that thread show that it’s not a design oversight, using a single anchor has drawbacks as well. But it’s quite off-topic for this PR though.

DrahtBot commented at 9:52 am on July 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27522383453

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Jul 22, 2024

DrahtBot removed the label CI failed on Jul 22, 2024

in doc/release-notes-30352.md:5 in 8d956ccb86 outdated

0@@ -0,0 +1,10 @@
1+P2P and network changes
2+-----------------------
3+
4+- Pay To Anchor(P2A) are a new standard witness output type for spending,
5+  and new a recognised output template. This allows for a key-less anchor

achow101 commented at 7:29 pm on July 22, 2024:

In 8d956ccb86d70e2059a5e2ccd8e63b7f7c8e3842 “Add release note for P2A output feature”

nit: typos

0  and a newly recognised output template. This allows for key-less anchor

instagibbs commented at 7:45 pm on July 22, 2024:

done

achow101 commented at 7:31 pm on July 22, 2024: member

Concept ACK

The code, docs, and the OP sometimes use OP_TRUE and sometimes OP_1. I would prefer if everything could refer to that opcode in the same way as it can be confusing when switching between the two. I would prefer if we used the OP_1 notation since the reliance on unknown segwit version behavior is an integral part of this.

instagibbs renamed this:
~~policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending~~
policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending
on Jul 22, 2024

instagibbs force-pushed on Jul 22, 2024

instagibbs commented at 7:46 pm on July 22, 2024: member

@achow101 swapped in OP_1 for OP_TRUE everywhere :+1:

instagibbs force-pushed on Jul 22, 2024

in src/script/solver.cpp:171 in 93cd123175 outdated

165@@ -165,6 +166,11 @@ TxoutType Solver(const CScript& scriptPubKey, std::vector<std::vector<unsigned c
166             vSolutionsRet.push_back(std::move(witnessprogram));
167             return TxoutType::WITNESS_V1_TAPROOT;
168         }
169+        if (scriptPubKey.IsPayToAnchor()) {
170+            vSolutionsRet.push_back(std::vector<unsigned char>{(unsigned char)witnessversion});
171+            vSolutionsRet.push_back(std::move(witnessprogram));

theStack commented at 10:50 pm on July 22, 2024:

nit: these two lines could just be removed, as the solutions array’s purpose is to contain the extracted non-fixed parts of an output script. For a P2A output, no such parts exist, and there is imho no value in returning constants.

instagibbs commented at 1:42 am on July 23, 2024:

done

instagibbs commented at 1:16 pm on July 23, 2024:

took a bit more work after removing this as I needed to add its own CTxDestination, but fixed finally

theStack commented at 11:04 pm on July 22, 2024: contributor

Concept ACK

instagibbs force-pushed on Jul 23, 2024

DrahtBot added the label CI failed on Jul 23, 2024

DrahtBot commented at 2:14 am on July 23, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27780417773

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

instagibbs force-pushed on Jul 23, 2024

DrahtBot removed the label CI failed on Jul 23, 2024

in src/addresstype.h:101 in e492879ebe outdated

 96+
 97+    friend bool operator==(const PayToAnchor& a1, const PayToAnchor& a2) {
 98+        return true;
 99+    }
100+
101+    friend bool operator<(const PayToAnchor& a1, const PayToAnchor& a2) {

ismaelsadeeq commented at 3:13 pm on July 23, 2024:

Why is this false, shouldn’t this be true also?

instagibbs commented at 4:37 pm on July 23, 2024:

since they’re always equal, I suspect they can never be unequal? Perhaps I’m violating some C++ custom here?

ismaelsadeeq commented at 5:02 pm on July 23, 2024:

Oh yes since all instances are equal, no instance is less than another, so should indeed be false. maybe add a comment on that.

instagibbs commented at 10:37 am on July 24, 2024:

I actually just re-used WitnessUnknown’s own operators since they’ll work just fine.

in src/addresstype.h:104 in e492879ebe outdated

 99+    }
100+
101+    friend bool operator<(const PayToAnchor& a1, const PayToAnchor& a2) {
102+        return false;
103+    }
104+};

ismaelsadeeq commented at 4:33 pm on July 23, 2024:

Is there something that will prevent us from deriving PayToAnchor from WitnessUnknown?

0struct PayToAnchor: public WitnessUnknown
1{
2    PayToAnchor() : WitnessUnknown(1, {0x4e, 0x73}) {};
3    friend bool operator==(const PayToAnchor& a1, const PayToAnchor& a2) {
4        return true;
5    }
6    friend bool operator<(const PayToAnchor& a1, const PayToAnchor& a2) {
7        return true;
8    }
9};

This way we can reuse the WitnessUnknown operator() that should be reused.

 0diff --git a/src/addresstype.cpp b/src/addresstype.cpp
 1index bfcc6cc62a4..1b22484c85b 100644
 2--- a/src/addresstype.cpp
 3+++ b/src/addresstype.cpp
 4@@ -143,11 +143,6 @@ public:
 5         return CScript() << OP_1 << ToByteVector(tap);
 6     }
 7 
 8-    CScript operator()(const PayToAnchor& anchor) const
 9-    {
10-        return CScript() << OP_1 << std::vector<unsigned char>{0x4e, 0x73};
11-    }
12-
13     CScript operator()(const WitnessUnknown& id) const
14     {
15         return CScript() << CScript::EncodeOP_N(id.GetWitnessVersion()) << id.GetWitnessProgram();
16diff --git a/src/addresstype.h b/src/addresstype.h
17index ea59578f760..fdb2230d8ed 100644
18--- a/src/addresstype.h
19+++ b/src/addresstype.h
20@@ -90,18 +90,6 @@ struct WitnessV1Taproot : public XOnlyPubKey
21     explicit WitnessV1Taproot(const XOnlyPubKey& xpk) : XOnlyPubKey(xpk) {}
22 };
23 
24-struct PayToAnchor
25-{
26-    PayToAnchor() = default;
27-
28-    friend bool operator==(const PayToAnchor& a1, const PayToAnchor& a2) {
29-        return true;
30-    }
31-
32-    friend bool operator<(const PayToAnchor& a1, const PayToAnchor& a2) {
33-        return false;
34-    }
35-};
36 
37 //! CTxDestination subtype to encode any future Witness version
38 struct WitnessUnknown
39@@ -129,6 +117,18 @@ public:
40     }
41 };
42 
43+
44+struct PayToAnchor: public WitnessUnknown
45+{
46+    PayToAnchor() : WitnessUnknown(1, {0x4e, 0x73}) {};
47+    friend bool operator==(const PayToAnchor& a1, const PayToAnchor& a2) {
48+        return true;
49+    }
50+    friend bool operator<(const PayToAnchor& a1, const PayToAnchor& a2) {
51+        return true;
52+    }
53+};
54+
55 /**
56  * A txout script categorized into standard templates.
57  *  * CNoDestination: Optionally a script, no corresponding address.
58diff --git a/src/key_io.cpp b/src/key_io.cpp
59index 6e869de8a5b..bba10011c1f 100644
60--- a/src/key_io.cpp
61+++ b/src/key_io.cpp
62@@ -64,16 +64,6 @@ public:
63         ConvertBits<8, 5, true>([&](unsigned char c) { data.push_back(c); }, tap.begin(), tap.end());
64         return bech32::Encode(bech32::Encoding::BECH32M, m_params.Bech32HRP(), data);
65     }
66-
67-    std::string operator()(const PayToAnchor& anchor) const
68-    {
69-        std::vector<unsigned char> data = {1};
70-        std::vector<unsigned char> program = {0x4e, 0x73};
71-        data.reserve(1 + (2 * 8 + 4) / 5);
72-        ConvertBits<8, 5, true>([&](unsigned char c) { data.push_back(c); }, program.begin(), program.end());
73-        return bech32::Encode(bech32::Encoding::BECH32M, m_params.Bech32HRP(), data);
74-    }
75-
76     std::string operator()(const WitnessUnknown& id) const
77     {
78         const std::vector<unsigned char>& program = id.GetWitnessProgram();

instagibbs commented at 10:36 am on July 24, 2024:

done, with even fewer lines. Nice to reduce the boilerplate a bit.

in test/functional/mempool_accept.py:405 in c8ed6702a5 outdated

400+        true_tx_spend = CTransaction()
401+        true_tx_spend.vin.append(CTxIn(COutPoint(true_tx.sha256, 0), b""))
402+        true_tx_spend.vout.append(CTxOut(true_tx.vout[0].nValue - int(fee*COIN), script_to_p2wsh_script(CScript([OP_TRUE]))))
403+        true_tx_spend.wit.vtxinwit.append(CTxInWitness())
404+        true_tx_spend.wit.vtxinwit[0].scriptWitness.stack.append(b"f")
405+        true_tx_spend.rehash()

glozow commented at 8:48 am on July 24, 2024:

nit on naming: I assume you originally named these after the script. Maybe call them tx_anchor, tx_spend_nonempty_wit, tx_spend_nested, etc?

instagibbs commented at 10:36 am on July 24, 2024:

renamed things

in test/functional/mempool_accept.py:397 in c8ed6702a5 outdated

389@@ -389,6 +390,56 @@ def run_test(self):
390             maxfeerate=0,
391         )
392 
393+        self.log.info('OP_1 <0x4e73> is able to be created and spent')
394+        true_tx = self.wallet.create_self_transfer(sequence=SEQUENCE_FINAL)['tx']
395+        true_tx.vout[0].scriptPubKey = bytes(PAY_TO_ANCHOR)
396+        true_tx.rehash()
397+        self.generateblock(node, self.wallet.get_address(), [true_tx.serialize().hex()])

glozow commented at 8:54 am on July 24, 2024:

This isn’t testing that mempool accepts a tx creating a p2a, assuming that’s what you were going for. Perhaps we want something like this (passes for me on master as well, as expected):

0        true_tx = self.wallet.create_self_transfer_multi(sequence=SEQUENCE_FINAL, num_outputs=2)['tx']
1        true_tx.vout[0].scriptPubKey = bytes(PAY_TO_ANCHOR)
2        node.sendrawtransaction(true_tx.serialize().hex()
3        self.generate(node, 1)

instagibbs commented at 10:36 am on July 24, 2024:

I actually didn’t mean to test it, but makes sense to in case by defining the output we somehow made it non-standard. Taken

instagibbs force-pushed on Jul 24, 2024

in test/functional/mempool_accept.py:397 in adbbfa4f3c outdated

389@@ -389,6 +390,58 @@ def run_test(self):
390             maxfeerate=0,
391         )
392 
393+        self.log.info('OP_1 <0x4e73> is able to be created and spent')
394+        create_anchor_tx = self.wallet.create_self_transfer_multi(sequence=SEQUENCE_FINAL, num_outputs=2)['tx']
395+        create_anchor_tx.vout[0].scriptPubKey = bytes(PAY_TO_ANCHOR)
396+        create_anchor_tx.rehash()
397+        node.sendrawtransaction(create_anchor_tx.serialize().hex())

theStack commented at 10:10 pm on July 24, 2024:

nit, could alternatively use MiniWallet’s .send_to method for less code (assuming that specifying nSequence explicitly is not important), e.g.:

 0diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
 1index cf3185271c..4b9ff3d138 100755
 2--- a/test/functional/mempool_accept.py
 3+++ b/test/functional/mempool_accept.py
 4@@ -391,16 +391,14 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
 5         )
 6 
 7         self.log.info('OP_1 <0x4e73> is able to be created and spent')
 8-        create_anchor_tx = self.wallet.create_self_transfer_multi(sequence=SEQUENCE_FINAL, num_outputs=2)['tx']
 9-        create_anchor_tx.vout[0].scriptPubKey = bytes(PAY_TO_ANCHOR)
10-        create_anchor_tx.rehash()
11-        node.sendrawtransaction(create_anchor_tx.serialize().hex())
12+        anchor_value = 10000
13+        create_anchor_tx = self.wallet.send_to(from_node=node, scriptPubKey=PAY_TO_ANCHOR, amount=anchor_value)
14         self.generate(node, 1)
15 
16         # First spend has non-empty witness, will be rejected to prevent third party wtxid malleability
17         anchor_nonempty_wit_spend = CTransaction()
18-        anchor_nonempty_wit_spend.vin.append(CTxIn(COutPoint(create_anchor_tx.sha256, 0), b""))
19-        anchor_nonempty_wit_spend.vout.append(CTxOut(create_anchor_tx.vout[0].nValue - int(fee*COIN), script_to_p2wsh_script(CScript([OP_TRUE]))))
20+        anchor_nonempty_wit_spend.vin.append(CTxIn(COutPoint(int(create_anchor_tx["txid"], 16), create_anchor_tx["sent_vout"]), b""))
21+        anchor_nonempty_wit_spend.vout.append(CTxOut(anchor_value - int(fee*COIN), script_to_p2wsh_script(CScript([OP_TRUE]))))
22         anchor_nonempty_wit_spend.wit.vtxinwit.append(CTxInWitness())
23         anchor_nonempty_wit_spend.wit.vtxinwit[0].scriptWitness.stack.append(b"f")
24         anchor_nonempty_wit_spend.rehash()

instagibbs commented at 12:04 pm on July 25, 2024:

will do if I touch the PR again

instagibbs commented at 5:34 pm on July 25, 2024:

done

in src/script/sign.cpp:415 in 7f13de02a9 outdated

411@@ -412,6 +412,7 @@ static bool SignStep(const SigningProvider& provider, const BaseSignatureCreator
412     case TxoutType::NONSTANDARD:
413     case TxoutType::NULL_DATA:
414     case TxoutType::WITNESS_UNKNOWN:
415+    case TxoutType::ANCHOR:
416         return false;

theStack commented at 10:27 pm on July 24, 2024:

obviously way out of scope for this PR, but I wonder: could there be scenarios where it’s useful/needed to have “signing support” for P2A outputs, by doing nothing and just succeed here? (probably not, as createrawtransaction with the P2A input is already sufficient to spend it).

instagibbs commented at 1:44 pm on July 25, 2024:

mapping out what is needed for more tooling seems like reasonable follow-up work, e.g. I can’t seem to populate a PSBT’s input for P2A unless the P2A output is already in the mempool (precludes pre-signing a CPFP package and submitting to mempool at same time). Of course you can use external tooling to do so.

sdaftuar commented at 3:48 pm on July 30, 2024:

Is there a downside to returning true in the case of TxoutType::ANCHOR here? eg I can’t see any tests fail when I make that change, so not clear to me what effect this has, or why this needs to be false.

instagibbs commented at 6:08 pm on July 30, 2024:

no good reason, though I feel like it’s going to be hard to hit given PSBT does script verification to check for finality.

Pushed the change, along with a trivial unit test covering ProduceSignature for this type of input.

theStack commented at 8:00 pm on July 30, 2024:

no good reason, though I feel like it’s going to be hard to hit given PSBT does script verification to check for finality.

I can confirm that at least for non-PSBT signing. Tried that on a tx with P2A input using signrawtransactionwithkey RPC, and it always succeeded, no matter whether the ANCHOR case in ProduceSignature returned true or false. So with the regular SignTransaction flow, script verification is done before signing as well (see DataFromTransaction), and it thus seems that at least currently, the discussed code is unreachable in production.

theStack approved

theStack commented at 10:32 pm on July 24, 2024: contributor

Code-review ACK 67c3537f75bcf085bb98e3830649e93da124cb06

Left a non-blocking test nit. In the future a dedicated MiniWallet-mode for P2A could probably make sense (should be trivial to implement).

DrahtBot requested review from ismaelsadeeq on Jul 24, 2024

DrahtBot requested review from glozow on Jul 24, 2024

DrahtBot requested review from t-bast on Jul 24, 2024

DrahtBot requested review from achow101 on Jul 24, 2024

in src/script/interpreter.cpp:1946 in 67c3537f75 outdated

1942@@ -1943,6 +1943,9 @@ static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion,
1943             }
1944             return set_success(serror);
1945         }
1946+    } else if (!is_p2sh && witversion == 1 && program.size() == 2 && program[0] == 0x4e && program[1] == 0x73) {

ismaelsadeeq commented at 4:00 pm on July 25, 2024:

nit: will be nice to define the program values 0x4e and 0x73 for PayToAnchor as const and reuse them, as the raw values appears in several places.

instagibbs commented at 5:34 pm on July 25, 2024:

I made another CScript helper function and tried to make things more self-documenting. Let me know what you think.

ismaelsadeeq commented at 5:41 pm on July 25, 2024:

looks much better to me 💯

ismaelsadeeq commented at 4:00 pm on July 25, 2024: member

Code review ACK 67c3537f75bcf085bb98e3830649e93da124cb06

instagibbs force-pushed on Jul 25, 2024

ismaelsadeeq commented at 5:43 pm on July 25, 2024: member

re-ACK e7ce6dc070c0319cbb868d41cadd836b2e6ca9db via diff

DrahtBot requested review from theStack on Jul 25, 2024

theStack approved

theStack commented at 9:15 pm on July 25, 2024: contributor

re-ACK e7ce6dc070c0319cbb868d41cadd836b2e6ca9db :moneybag: :two: :anchor:

sdaftuar commented at 5:33 pm on July 30, 2024: member

Code review ACK e7ce6dc070c0319cbb868d41cadd836b2e6ca9db

I mostly reviewed this for correctness, and have only given a little bit of thought to the design decisions here – but the reasoning provided for the choice of this script makes sense to me.

policy: Add OP_1 <0x4e73> as a standard output type

These outputs are called anchors, and allow
key-less anchor spends which are vsize-minimized
versus keyed anchors which require larger outputs
when creating and inputs when spending.

455fca86cf

policy: make anchor spend standard

Only standard when non-nested.

b60aaf8b23

policy: stop 3rd party wtxid malleability of anchor spend 9d89209937

test: Add anchor mempool acceptance test 1349e9ec15

test: add P2A coverage for decodescript 71c9b02a04

Add release note for P2A output feature 7998ce6b20

test: add P2A ProduceSignature coverage 75648cea5a

instagibbs force-pushed on Jul 30, 2024

in src/test/script_tests.cpp:1290 in 75648cea5a

1285+    CMutableTransaction prev, curr;
1286+    prev.vout.emplace_back(0, GetScriptForDestination(PayToAnchor{}));
1287+
1288+    curr.vin.emplace_back(COutPoint{prev.GetHash(), 0});
1289+
1290+    BOOST_CHECK(SignSignature(keystore, CTransaction(prev), curr, 0, SIGHASH_ALL, sig_data));

theStack commented at 8:04 pm on July 30, 2024:

Kind of unrelated follow-up material comment: these SignSignature functions are a bit odd, as they are never used anywhere outside of test code. Maybe they can be removed and replaced with something else in the tests in the future.

instagibbs commented at 8:09 pm on July 30, 2024:

Yes I noticed this, they can probably be moved to utils directory at a minimum?

theStack approved

theStack commented at 8:07 pm on July 30, 2024: contributor

re-ACK 75648cea5a9032b3d388cbebacb94d908e08924e

Only change since last ACK is changing the return value of ProduceSignature for P2A inputs to true/success (which doesn’t seem to matter in practice, see #30352 (review)) and a new unit test checking this.

DrahtBot requested review from sdaftuar on Jul 30, 2024

DrahtBot requested review from ismaelsadeeq on Jul 30, 2024

sdaftuar commented at 11:07 pm on July 30, 2024: member

utACK 75648cea5a9032b3d388cbebacb94d908e08924e

in src/script/solver.cpp:170 in 455fca86cf outdated

165@@ -165,6 +166,9 @@ TxoutType Solver(const CScript& scriptPubKey, std::vector<std::vector<unsigned c
166             vSolutionsRet.push_back(std::move(witnessprogram));
167             return TxoutType::WITNESS_V1_TAPROOT;
168         }
169+        if (scriptPubKey.IsPayToAnchor()) {
170+            return TxoutType::ANCHOR;

glozow commented at 12:53 pm on July 31, 2024:

Noting that nothing is pushed to vSolutionsRet here, but that makes sense / callers are not assuming that the program should be pushed.

in src/addresstype.h:120 in 455fca86cf outdated

116@@ -116,6 +117,13 @@ struct WitnessUnknown
117     }
118 };
119 
120+struct PayToAnchor : public WitnessUnknown

glozow commented at 1:02 pm on July 31, 2024:

Question: It probably doesn’t matter because of this inheritance relationship, but should there be a bool operator()(const PayToAnchor& dest) const { return true; } in class ValidDestinationVisitor?

instagibbs commented at 2:58 pm on July 31, 2024:

it’s functionally equivalent and this way we avoid additional boilerplate, leaving as is

ismaelsadeeq commented at 1:14 pm on July 31, 2024: member

re-ACK 75648cea5a9032b3d388cbebacb94d908e08924e via diff

in src/rpc/util.cpp:339 in 455fca86cf outdated

331@@ -332,6 +332,14 @@ class DescribeAddressVisitor
332         return obj;
333     }
334 
335+    UniValue operator()(const PayToAnchor& anchor) const
336+    {
337+        UniValue obj(UniValue::VOBJ);
338+        obj.pushKV("isscript", true);
339+        obj.pushKV("iswitness", true);

glozow commented at 1:34 pm on July 31, 2024:

Why not push anchor.GetWitnessVersion() and anchor.GetWitnessProgram() here?

instagibbs commented at 2:31 pm on July 31, 2024:

it’s a static value, so I’m unsure what the benefits are

glozow commented at 3:24 pm on July 31, 2024:

Just to provide everything for getaddressinfo results. I suppose they’re optional fields though, nbd

instagibbs commented at 5:32 pm on July 31, 2024:

we can always add it later if we decide it’s better, resolving for now

in test/functional/rpc_decodescript.py:190 in 71c9b02a04 outdated

186@@ -187,6 +187,16 @@ def decodescript_script_pub_key(self):
187         assert_equal('1 ' + xonly_public_key, rpc_result['asm'])
188         assert 'segwit' not in rpc_result
189 
190+        self.log.info("- P2A (anchor)")

glozow commented at 1:47 pm on July 31, 2024:

fwiw I don’t see any coverage of the “bc1pfeessrawgf” string. If desired, could add to rpc_validateaddress.py

0    (
1        "bc1pfeessrawgf",
2        "51024e73",
3    )

instagibbs commented at 2:53 pm on July 31, 2024:

will do in follow-up

instagibbs commented at 5:32 pm on July 31, 2024:

added to followup pr https://github.com/bitcoin/bitcoin/pull/30562

in test/functional/mempool_accept.py:440 in 1349e9ec15 outdated

435+        self.check_mempool_result(
436+            result_expected=[{'txid': nested_anchor_spend.rehash(), 'allowed': False, 'reject-reason': 'non-mandatory-script-verify-flag (Witness version reserved for soft-fork upgrades)'}],
437+            rawtxs=[nested_anchor_spend.serialize().hex()],
438+            maxfeerate=0,
439+        )
440+        # but is consensus-legal

glozow commented at 1:59 pm on July 31, 2024:

Would have been nice for these 3 test cases to be a bit more independent, and for consensus-legality test to be on the witness-stuffed tx too.

instagibbs commented at 2:53 pm on July 31, 2024:

good idea, will do a follow-up

instagibbs commented at 5:32 pm on July 31, 2024:

added to followup pr https://github.com/bitcoin/bitcoin/pull/30562

glozow commented at 2:03 pm on July 31, 2024: member

ACK 75648cea5a9032b3d388cbebacb94d908e08924e

in src/script/solver.h:25 in 455fca86cf outdated

21@@ -22,6 +22,7 @@ template <typename C> class Span;
22 enum class TxoutType {
23     NONSTANDARD,
24     // 'standard' transaction types:
25+    ANCHOR, //!< anyone can spend script

tdb3 commented at 11:33 am on August 2, 2024:

nit: If I’m reading the history correctly, there was some initial conversation of a non-segwit ephemeral anchor (e.g. just OP_TRUE (and with no empty scriptsig guarantees that segwit offers), for situations not caring much for malleability resistance). This segwit anchor seems like a much better way to go, but I’m not sure if other anchor types are still being considered (and nothing sticks out particularly as a reason why we would even really need more than one type).

Would this be the only anticipated anchor type to be introduced? If so, ANCHOR seems reasonable. If not, then the enum (and locations using it) could use a different name to be more specific.

instagibbs commented at 1:36 pm on August 2, 2024:

I hadn’t considered txid malleability at the time, because:

I forgot how bad legacy script is
I hadn’t considered scenarios like: LN splice pays for LN unilateral close, which would rely on txid stability of the child

Once I realized (2) I was annoyed for a half a year until I realized the solution.

I have no plans to add in a malleable version for now, and we can change this later even if we do.

in src/test/script_standard_tests.cpp:132 in 455fca86cf outdated

127@@ -128,6 +128,20 @@ BOOST_AUTO_TEST_CASE(script_standard_Solver_success)
128     BOOST_CHECK(solutions[0] == std::vector<unsigned char>{16});
129     BOOST_CHECK(solutions[1] == ToByteVector(uint256::ONE));
130 
131+    // TxoutType::ANCHOR
132+    std::vector<unsigned char> anchor_bytes{0x4e, 0x73};

tdb3 commented at 11:40 am on August 2, 2024:

nit: Rather than repeating 0x4e, 0x73 in multiple places, maybe this could be macro’d out (or similar)? At minimum, this might help increase clarity and reduce adding explanatory comments for the magic numbers. Also seems more consistent with DRY principle, but I doubt these values will ever change (so maybe not a ton of value-added). Alternatively, could be done in a follow-up PR.

instagibbs commented at 1:38 pm on August 2, 2024:

I’d be willing to entertain code changes in followups for this. Leaving as-is.

in test/functional/mempool_accept.py:393 in 1349e9ec15 outdated

389@@ -389,6 +390,56 @@ def run_test(self):
390             maxfeerate=0,
391         )
392 
393+        self.log.info('OP_1 <0x4e73> is able to be created and spent')

tdb3 commented at 11:48 am on August 2, 2024:

Could be beneficial to add tests for P2A interaction with V2/V3 transactions and RBF scenarios. Probably best for a follow-up.

instagibbs commented at 1:39 pm on August 2, 2024:

Seems more like a (useful) demo to me. I have old branches that mashed TRUC/Ephemeral Dust/Anchors/etc together to show the use-cases, but seems out of scope for these tests now that the features are separated.

in doc/release-notes-30352.md:5 in 75648cea5a

0@@ -0,0 +1,10 @@
1+P2P and network changes
2+-----------------------
3+
4+- Pay To Anchor(P2A) is a new standard witness output type for spending,
5+  a newly recognised output template. This allows for key-less anchor

tdb3 commented at 11:56 am on August 2, 2024:

nitty nit: Although I think it’s perfectly clear, I’m not 100% sure output template fully aligns with the terminology BIP (https://github.com/murchandamus/bips/blob/2022-04-tx-terminology/bip-tx-terminology.mediawiki). Maybe script template?

instagibbs commented at 1:42 pm on August 2, 2024:

not sure it’s worth the churn, sorry :) leaving as-is

tdb3 approved

tdb3 commented at 11:59 am on August 2, 2024: contributor

ACK 75648cea5a9032b3d388cbebacb94d908e08924e Great stuff. Nice elegant solution for anchors, re-using segwit malleability resistance and a minimal byte footprint. Left a few comments/nits, but none block the ACK.

glozow merged this on Aug 2, 2024

glozow closed this on Aug 2, 2024

policy: Add PayToAnchor(P2A), OP_1 <0x4e73> as a standard output script for spending #30352

Code Coverage

Reviews

Conflicts

policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending #30352