Use serialization parameters for CTransaction

ajtowns commented at 4:41 am on September 9, 2023: contributor

Choose whether witness is included in transaction serialization via serialization parameter rather than the stream version. See #25284 and #19477 for previous context.

DrahtBot commented at 4:41 am on September 9, 2023: 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	maflcko, theuni
Approach ACK	sipa

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:

#28311 ([WIP] BIP300 (Drivechains) consensus-level logic by luke-jr)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#26415 (rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block by andrewtoth)
#21283 (Implement BIP 370 PSBTv2 by achow101)

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.

in src/consensus/validation.h:164 in 75fde10d77 outdated

159@@ -160,7 +160,8 @@ static inline int64_t GetTransactionInputWeight(const CTxIn& txin)
160 }
161 static inline int64_t GetTransactionOutputWeight(const CTxOut& txout)
162 {
163-    return ::GetSerializeSize(txout, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(txout, PROTOCOL_VERSION);
164+    // CTxOut does not include witness data, so does not need to be serialized twice
165+    return ::GetSerializeSize(txout) * WITNESS_SCALE_FACTOR;

ajtowns commented at 4:42 am on September 9, 2023:

Probably obsoleted by #28431

ajtowns force-pushed on Sep 9, 2023

DrahtBot added the label CI failed on Sep 9, 2023

DrahtBot added the label Needs rebase on Sep 9, 2023

ajtowns force-pushed on Sep 9, 2023

DrahtBot removed the label Needs rebase on Sep 9, 2023

DrahtBot removed the label CI failed on Sep 10, 2023

sipa commented at 10:17 pm on September 10, 2023: member

It doesn’t seem crazy to me to drop -rpcserialversion and/or replace it with RPC specific parameters, but we should probably deprecate it first, no? We could try to do that in 26.0, and then continue with the changes in this PR for the release after the branching.

ajtowns commented at 7:27 am on September 11, 2023: contributor

It doesn’t seem crazy to me to drop -rpcserialversion and/or replace it with RPC specific parameters, but we should probably deprecate it first, no? We could try to do that in 26.0, and then continue with the changes in this PR for the release after the branching.

(Isn’t that what I suggested in the OP? If there’s a nuance, I’ve missed it)

I’ve opened up a PR to deprecate -rpcserialversion=0 in 26.0 at #28448.

ajtowns added this to the milestone 27.0 on Sep 11, 2023

maflcko commented at 7:40 am on September 11, 2023: member

This is mainly because that uses RPCSerializationFlags() and rpcSerializationFlags() interfaces which don’t really match the new model.

I don’t understand this. It should be trivial to return a TransactionSerParams from RPCSerializationFlags, no?

It seems fine to add an option to RPC methods to disable witness, but shouldn’t that be done before removing/deprecating the global setting?

in src/bench/checkblock.cpp:27 in 91a814a91f outdated

23@@ -24,7 +24,7 @@ static void DeserializeBlockTest(benchmark::Bench& bench)
24 
25     bench.unit("block").run([&] {
26         CBlock block;
27-        stream >> block;
28+        stream >> WithParams(TX_WITH_WITNESS, block);

maflcko commented at 7:46 am on September 11, 2023:

Why not

0        stream >> TX_WITH_WITNESS(block);

? See #25284 (comment)

ajtowns commented at 7:59 am on September 11, 2023:

Because I wrote this code before having that thought :)

ajtowns commented at 10:40 am on September 11, 2023:

Pushed a scripted-diff to switch to this style

ajtowns commented at 8:27 am on September 11, 2023: contributor

I don’t understand this. It should be trivial to return a TransactionSerParams from RPCSerializationFlags, no?

It’s not clear to me that it would make sense to do that for interfaces::Chain::rpcSerializationFlags, and it’s not clear to me that there’s any ongoing value in -rpcserialversion to make it even worth thinking through… so I figured better to just put something up for discussion.

in src/primitives/transaction.h:220 in acf66c4dcc outdated

212@@ -214,8 +213,15 @@ struct CMutableTransaction;
213  * - uint32_t nLockTime
214  */
215 template<typename Stream, typename TxType>
216-inline void UnserializeTransaction(TxType& tx, Stream& s) {
217-    const bool fAllowWitness = !(s.GetVersion() & SERIALIZE_TRANSACTION_NO_WITNESS);
218+inline void UnserializeTransaction(TxType& tx, Stream& s)
219+{
220+    UnserializeTransaction(tx, s, s.GetParams());

maflcko commented at 12:35 pm on September 11, 2023:

I wonder if it makes sense to split this up, to allow moving the p2p layer first, and leaving the rpc layer as-is for now?

Maybe this can be done by adding a property to CDataStream and DataStream to indicate whether they are legacy or not?

0
1if constexpr (s.IS_LEGACY) {
2  UnserializeTransaction(tx, s, s.GetVersion() & SERIALIZE_TRANSACTION_NO_WITNESS ? NO_WIT : YO_WIT);
3} else {
4  UnserializeTransaction(tx, s, s.GetParams());
5}

In any case, you forgot to replace CDataStream in net with DataStream?

ajtowns commented at 11:05 pm on September 11, 2023:

I think it’d be better to just update the rpc layer if that behaviour’s worth keeping.

I had a look at updating CDataStream in net but didn’t think it was worth including in this draft – I don’t think there’s anything subtle/bikesheddy about it that warrants much discussion, and it touches the V2 stuff so seemed like it would just prompt extra rebasing.

DrahtBot added the label Needs rebase on Sep 12, 2023

maflcko commented at 12:04 pm on September 13, 2023: member

Would it make sense to split up the last commit from this pull, along with your two suggested follow-ups in #25284 (review) and #25284 (review) ?

ajtowns commented at 0:31 am on September 14, 2023: contributor

Would it make sense to split up the last commit from this pull, along with your two suggested follow-ups in #25284 (comment) and #25284 (comment) ?

I think so. Done in #28473. (If you wanted to do it, possibly with other cleanups, feel free to consider it up-for-grabs)

ajtowns force-pushed on Sep 21, 2023

DrahtBot removed the label Needs rebase on Sep 21, 2023

ajtowns commented at 4:33 am on September 21, 2023: contributor

Rebased on top of master, but not #28503 or #28508, reviewers might like to look at those first.

DrahtBot added the label CI failed on Sep 24, 2023

ajtowns force-pushed on Sep 26, 2023

ajtowns commented at 4:34 am on September 26, 2023: contributor

Rebased after #28492; changes int RPCSerializationFlags() to bool RPCSerializationWithoutWitness() so that -rpcserialiversion doesn’t have to be dropped immediately, replaces some CDataStream with DataStream.

sipa commented at 2:24 pm on September 26, 2023: member

Concept/Approach ACK. I prefer being able to make this change without it necessarily being tied to dropping -rpcserialversion support.

DrahtBot added the label Needs rebase on Oct 2, 2023

ajtowns force-pushed on Oct 2, 2023

ajtowns commented at 12:37 pm on October 2, 2023: contributor

Rebased over #28508 ; reviewers may still wish to check #28503 prior to this one

DrahtBot removed the label Needs rebase on Oct 2, 2023

DrahtBot removed the label CI failed on Oct 2, 2023

DrahtBot added the label Needs rebase on Oct 16, 2023

ajtowns force-pushed on Oct 17, 2023

ajtowns commented at 2:43 am on October 17, 2023: contributor

Rebased over #28539

DrahtBot added the label CI failed on Oct 17, 2023

DrahtBot removed the label Needs rebase on Oct 17, 2023

DrahtBot removed the label CI failed on Oct 19, 2023

DrahtBot added the label Needs rebase on Oct 26, 2023

ajtowns force-pushed on Oct 27, 2023

ajtowns marked this as ready for review on Oct 27, 2023

ajtowns commented at 4:49 am on October 27, 2023: contributor

Rebased over #28107 and un-drafted now that 26.x is branched off.

DrahtBot removed the label Needs rebase on Oct 27, 2023

DrahtBot added the label CI failed on Nov 1, 2023

ajtowns force-pushed on Nov 1, 2023

in src/bench/checkblock.cpp:27 in 1573f2c88c outdated

23@@ -24,7 +24,7 @@ static void DeserializeBlockTest(benchmark::Bench& bench)
24 
25     bench.unit("block").run([&] {
26         CBlock block;
27-        stream >> block;
28+        stream >> TX_WITH_WITNESS(block);

maflcko commented at 9:42 am on November 1, 2023:

review note: I presume reviewers are supposed to set --function-context and verify each time that SERIALIZE_TRANSACTION_NO_WITNESS was never embedded into the CDataStream in each context.

maflcko commented at 10:18 am on November 1, 2023:

But then you are changing it to DataStream in src/core_read.cpp, so why not here, at least in functions where the DataStream is created in the same function context?

ajtowns commented at 11:44 am on November 1, 2023:

I changed to the newer streams in places where I noticed it was obviously sensible, but didn’t try to be thorough about it, since being complete required extra changes like #28451. The fact that SERIALIZE_TRANSACTION_NO_WITNESS no longer exists means that any previous (explicit) embedding of it will have been touched by this changeset, fwiw.

in src/blockencodings.h:52 in 1573f2c88c outdated

48@@ -49,7 +49,7 @@ class BlockTransactionsRequest {
49 
50     SERIALIZE_METHODS(BlockTransactionsRequest, obj)
51     {
52-        READWRITE(obj.blockhash, Using<VectorFormatter<DifferenceFormatter>>(obj.indexes));
53+        READWRITE(obj.blockhash, TX_WITH_WITNESS(Using<VectorFormatter<DifferenceFormatter>>(obj.indexes)));

maflcko commented at 9:42 am on November 1, 2023:

??? What is this?

maflcko commented at 9:45 am on November 1, 2023:

(uint16_t is different from CTransaction)

fanquake requested review from TheCharlatan on Nov 1, 2023

in src/blockencodings.h:79 in 1573f2c88c outdated

75@@ -76,7 +76,7 @@ struct PrefilledTransaction {
76     uint16_t index;
77     CTransactionRef tx;
78 
79-    SERIALIZE_METHODS(PrefilledTransaction, obj) { READWRITE(COMPACTSIZE(obj.index), Using<TransactionCompression>(obj.tx)); }
80+    SERIALIZE_METHODS(PrefilledTransaction, obj) { READWRITE(COMPACTSIZE(obj.index), TX_WITH_WITNESS(Using<TransactionCompression>(obj.tx))); }

maflcko commented at 10:13 am on November 1, 2023:

This compiles, so I guess it is fine. Two notes:

This forces bip152-v2-only in the serialize code. I guess it is fine, given #20799.
question: If TransactionCompression was ever implemented it wouldn’t support serialization without witness? So wouldn’t it be cleaner to just implement the TransactionCompression formatter as one that calls (Un)SerializeTransaction with the witness params set?

ajtowns commented at 11:58 am on November 1, 2023:

I suspect it’d be nicer to have PushMessage(pfrom, msgMaker.Make(BLOCKTXN, BIP_XXX_TX_COMPRESSION(resp))) so that net_processing can turn compression on based on whether it’s been negotiated or not, and drop TransactionCompression entirely? Seems out of scope for this PR either way.

maflcko commented at 1:36 pm on November 1, 2023:

I suspect it’d be nicer to have PushMessage(pfrom, msgMaker.Make(BLOCKTXN, BIP_XXX_TX_COMPRESSION(resp))) so that net_processing can turn compression on based on whether it’s been negotiated or not

In that case, compression would be using ser params, so the witness flag would also have to be passed down via net_processing? So it seems better to set the witness flag in net processing for compact blocks and blocktxn?

ajtowns commented at 2:16 pm on November 1, 2023:

Not clear – you could have BIP_XXX_TX_COMPRESSION be a BlockTransactionsSerParams and have the logic for compression-or-not happen in BlockTransactions::Serialize, or it could be a separate formatter. In either case the TX_WITH_WITNESS flag could go in with the logic there still, I think.

in src/consensus/tx_check.cpp:19 in 1573f2c88c outdated

15@@ -16,7 +16,7 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
16     if (tx.vout.empty())
17         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-vout-empty");
18     // Size limits (this doesn't take the witness into account, as that hasn't been checked for malleability)
19-    if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT)
20+    if (::GetSerializeSize(TX_NO_WITNESS(tx)) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT)

maflcko commented at 10:15 am on November 1, 2023:

style nit: Forgot to add {} when touching this line?

in src/kernel/mempool_persist.cpp:77 in 1573f2c88c outdated

66@@ -67,7 +67,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
67             CTransactionRef tx;
68             int64_t nTime;
69             int64_t nFeeDelta;
70-            file >> tx;
71+            file >> TX_WITH_WITNESS(tx);

maflcko commented at 10:29 am on November 1, 2023:

Cautofile -> AutoFile?

in src/net_processing.cpp:5699 in 1573f2c88c outdated

5695@@ -5696,7 +5696,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
5696                         LogPrint(BCLog::NET, "%s: sending header %s to peer=%d\n", __func__,
5697                                 vHeaders.front().GetHash().ToString(), pto->GetId());
5698                     }
5699-                    m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::HEADERS, vHeaders));
5700+                    m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders)));  // ditto

maflcko commented at 10:35 am on November 1, 2023:

Not sure if the comments adds value to someone reading the lines of code here?

ajtowns commented at 12:53 pm on November 1, 2023:

That ditto one sure doesn’t… Removed them both.

DrahtBot removed the label CI failed on Nov 1, 2023

in src/rpc/blockchain.cpp:170 in 1573f2c88c outdated

165@@ -166,8 +166,8 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn
166 {
167     UniValue result = blockheaderToJSON(tip, blockindex);
168 
169-    result.pushKV("strippedsize", (int)::GetSerializeSize(block, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS));
170-    result.pushKV("size", (int)::GetSerializeSize(block, PROTOCOL_VERSION));
171+    result.pushKV("strippedsize", (int)::GetSerializeSize(TX_NO_WITNESS(block)));
172+    result.pushKV("size", (int)::GetSerializeSize(TX_WITH_WITNESS(block), PROTOCOL_VERSION));

maflcko commented at 10:51 am on November 1, 2023:

You’ll have to remove PROTOCOL_VERSION now. Also, GetSerializeSize could drop the int param completely?

ajtowns commented at 12:49 pm on November 1, 2023:

Also, GetSerializeSize could drop the int param completely?

Looks like; but seems a bit much for this PR. See https://github.com/ajtowns/bitcoin/commits/202310-getserializesize

in src/rpc/blockchain.cpp:169 in 1573f2c88c outdated

165@@ -166,8 +166,8 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn
166 {
167     UniValue result = blockheaderToJSON(tip, blockindex);
168 
169-    result.pushKV("strippedsize", (int)::GetSerializeSize(block, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS));
170-    result.pushKV("size", (int)::GetSerializeSize(block, PROTOCOL_VERSION));
171+    result.pushKV("strippedsize", (int)::GetSerializeSize(TX_NO_WITNESS(block)));

maflcko commented at 10:52 am on November 1, 2023:

any reason for the c-style cast? Why not remove it completely?

ajtowns commented at 12:59 pm on November 1, 2023:

No idea; seems easier to review without additional changes, though?

in src/test/fuzz/script_assets_test_minimizer.cpp:71 in 1573f2c88c outdated

67@@ -68,7 +68,7 @@ std::vector<CTxOut> TxOutsFromJSON(const UniValue& univalue)
68     for (size_t i = 0; i < univalue.size(); ++i) {
69         CTxOut txout;
70         try {
71-            SpanReader{0, CheckedParseHex(univalue[i].get_str())} >> txout;
72+            SpanReader{0, CheckedParseHex(univalue[i].get_str())} >> TX_WITH_WITNESS(txout);

maflcko commented at 11:04 am on November 1, 2023:

??

in src/test/fuzz/primitives_transaction.cpp:19 in 1573f2c88c outdated

15@@ -16,16 +16,16 @@ FUZZ_TARGET(primitives_transaction)
16 {
17     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
18     const CScript script = ConsumeScript(fuzzed_data_provider);
19-    const std::optional<COutPoint> out_point = ConsumeDeserializable<COutPoint>(fuzzed_data_provider);
20+    const std::optional<COutPoint> out_point = ConsumeDeserializable<COutPoint>(fuzzed_data_provider, TX_WITH_WITNESS);

maflcko commented at 11:04 am on November 1, 2023:

???

in src/test/fuzz/primitives_transaction.cpp:28 in 1573f2c88c outdated

26     const CTxOut tx_out_2{ConsumeMoney(fuzzed_data_provider), ConsumeScript(fuzzed_data_provider)};
27     assert((tx_out_1 == tx_out_2) != (tx_out_1 != tx_out_2));
28-    const std::optional<CMutableTransaction> mutable_tx_1 = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider);
29-    const std::optional<CMutableTransaction> mutable_tx_2 = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider);
30+    const std::optional<CMutableTransaction> mutable_tx_1 = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);
31+    const std::optional<CMutableTransaction> mutable_tx_2 = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);

maflcko commented at 11:14 am on November 1, 2023:

ConsumeDeserializable is never called with witness deserialization disabled. I wonder if it makes sense to offer a shorter version with the param set as default?

ajtowns commented at 12:10 pm on November 1, 2023:

Maybe? Being explicit seems not that bad though?

maflcko approved

maflcko commented at 11:14 am on November 1, 2023: member

lgtm, left some style questions.

lgtm ACK e5cb04009095c21517be71c0bf21cde4ed0490a7 📿

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK e5cb04009095c21517be71c0bf21cde4ed0490a7 📿
3Guc1h7mELOQ1nATm0THeRfCMOsdUXxt0uYEbrY3tUkevK/PO+Z0YDoAFIyYNveom27Om+aAiNBS+zKB4t9LEBg==

DrahtBot requested review from sipa on Nov 1, 2023

in src/primitives/transaction.h:224 in e5cb040090 outdated

221+{
222+    UnserializeTransaction(tx, s, s.GetParams());
223+}
224+
225+template<typename Stream, typename TxType>
226+inline void UnserializeTransaction(TxType& tx, Stream& s, const TransactionSerParams& params)

maflcko commented at 12:08 pm on November 1, 2023:

nit: template implies inline

in src/primitives/transaction.h:266 in e5cb040090 outdated

260@@ -254,8 +261,15 @@ inline void UnserializeTransaction(TxType& tx, Stream& s) {
261 }
262 
263 template<typename Stream, typename TxType>
264-inline void SerializeTransaction(const TxType& tx, Stream& s) {
265-    const bool fAllowWitness = !(s.GetVersion() & SERIALIZE_TRANSACTION_NO_WITNESS);
266+inline void SerializeTransaction(const TxType& tx, Stream& s)
267+{
268+    SerializeTransaction(tx, s, s.GetParams());

maflcko commented at 12:09 pm on November 1, 2023:

nit: (same above), given that this function is only for internal use for this module, and only used twice, is it needed?

in src/primitives/transaction.h:220 in e5cb040090 outdated

214@@ -215,8 +215,15 @@ struct CMutableTransaction;
215  * - uint32_t nLockTime
216  */
217 template<typename Stream, typename TxType>
218-inline void UnserializeTransaction(TxType& tx, Stream& s) {
219-    const bool fAllowWitness = !(s.GetVersion() & SERIALIZE_TRANSACTION_NO_WITNESS);
220+inline void UnserializeTransaction(TxType& tx, Stream& s)
221+{
222+    UnserializeTransaction(tx, s, s.GetParams());

maflcko commented at 12:11 pm on November 1, 2023:

nit: (same above), given that this function is only for internal use for this module, and only used once, is it needed?

ajtowns force-pushed on Nov 1, 2023

maflcko commented at 1:34 pm on November 1, 2023: member

re-ACK 1bbddaa060b1826466740f580a604eb3d87a28c0 🙍

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 1bbddaa060b1826466740f580a604eb3d87a28c0 🙍
3nugqspM/D8BnTd8Z2XWjI74maUhYx43behHeH8zEGzEmrW3AdSrRvnklZFgjl1PXWKP4tyw6TNB5oNehP0oIDw==

ajtowns force-pushed on Nov 1, 2023

ajtowns commented at 1:42 pm on November 1, 2023: contributor

Rebased over #28503 and addressed comments from @maflcko

maflcko commented at 1:45 pm on November 1, 2023: member

re-ACK ad132a1c4d592ca81f540637d5e7def76dd81c87 🕺

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK ad132a1c4d592ca81f540637d5e7def76dd81c87 🕺
3jOQCRBKv6qMg25zEo56CQbv9Het2GqLFz8JVHvCiYQKc89V2dNXpYKP3SxHa6fWZLvekK4TwUOpQePiPLrLaDQ==

in src/primitives/transaction.h:198 in ad132a1c4d outdated

193+{
194+    const bool allow_witness;
195+    SER_PARAMS_OPFUNC
196+};
197+static constexpr TransactionSerParams TX_WITH_WITNESS{.allow_witness=true};
198+static constexpr TransactionSerParams TX_NO_WITNESS{.allow_witness=false};

maflcko commented at 2:46 pm on November 1, 2023:

clang-format new code?

 0diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
 1index 1b188e0f49..d323a320b5 100644
 2--- a/src/primitives/transaction.h
 3+++ b/src/primitives/transaction.h
 4@@ -189,13 +189,12 @@ public:
 5 
 6 struct CMutableTransaction;
 7 
 8-struct TransactionSerParams
 9-{
10+struct TransactionSerParams {
11     const bool allow_witness;
12     SER_PARAMS_OPFUNC
13 };
14-static constexpr TransactionSerParams TX_WITH_WITNESS{.allow_witness=true};
15-static constexpr TransactionSerParams TX_NO_WITNESS{.allow_witness=false};
16+static constexpr TransactionSerParams TX_WITH_WITNESS{.allow_witness = true};
17+static constexpr TransactionSerParams TX_NO_WITNESS{.allow_witness = false};
18 
19 /**
20  * Basic transaction serialization format:

ajtowns commented at 8:46 pm on November 8, 2023:

Huh, I hadn’t realised we have different formats for class/struct.

fanquake requested review from theuni on Nov 8, 2023

DrahtBot added the label Needs rebase on Nov 8, 2023

ajtowns force-pushed on Nov 8, 2023

DrahtBot removed the label Needs rebase on Nov 8, 2023

DrahtBot added the label CI failed on Nov 8, 2023

DrahtBot removed the label CI failed on Nov 9, 2023

maflcko commented at 3:44 pm on November 9, 2023: member

re-ACK d0d4308750c14c44ed2a9b60bb44d6ec5423bd5a 💇

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK d0d4308750c14c44ed2a9b60bb44d6ec5423bd5a 💇
3W7OIAFLZyeG6udQ8cTBp0cfWedyHoACHlsbhwy+IQxob2O6zlcMNX6I/MhhtDU8eXKTmxIc9AnZidQXMa4EEDA==

DrahtBot added the label Needs rebase on Nov 13, 2023

Use ParamsWrapper for witness serialization 6e9e4e6130

Drop OverrideStream c94f7e5b1c

Drop CHashWriter a0c254c13a

ajtowns force-pushed on Nov 13, 2023

ajtowns commented at 10:54 pm on November 13, 2023: contributor

Rebased over #28207

DrahtBot removed the label Needs rebase on Nov 13, 2023

maflcko commented at 2:01 pm on November 14, 2023: member

re-ACK a0c254c13a3ef21e257cca3493446c632b636b15 🐜

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK a0c254c13a3ef21e257cca3493446c632b636b15 🐜
3zfOZlUyCFuozt+jcaewrWGPruTgfXkr8HvGVTeV28+8MWPaLDUFBRzsUDaI8ox0eHjTDmbdeLHedy/iZQiV3Dw==

in src/core_io.h:54 in 6e9e4e6130 outdated

50@@ -51,9 +51,9 @@ bool ParseHashStr(const std::string& strHex, uint256& result);
51 // core_write.cpp
52 UniValue ValueFromAmount(const CAmount amount);
53 std::string FormatScript(const CScript& script);
54-std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
55+std::string EncodeHexTx(const CTransaction& tx, const bool without_witness = false);

theuni commented at 7:35 pm on November 14, 2023:

It’s kinda a shame to carry this default param forward. But it’s also not much better to have a bool at each callsite. An enum class {witness/no_witness} would remove all ambiguity at the callsites, but that may be overkill.

ajtowns commented at 1:51 am on November 15, 2023:

That argument is only needed to support -rpcserialversion, which is deprecated as of #28448, so presumably can/will be pulled out later.

in src/core_io.h:57 in 6e9e4e6130 outdated

50@@ -51,9 +51,9 @@ bool ParseHashStr(const std::string& strHex, uint256& result);
51 // core_write.cpp
52 UniValue ValueFromAmount(const CAmount amount);
53 std::string FormatScript(const CScript& script);
54-std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
55+std::string EncodeHexTx(const CTransaction& tx, const bool without_witness = false);
56 std::string SighashToStr(unsigned char sighash_type);
57 void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex = true, bool include_address = false, const SigningProvider* provider = nullptr);
58-void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_DETAILS);
59+void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry, bool include_hex = true, bool without_witness = false, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_DETAILS);

theuni commented at 7:35 pm on November 14, 2023:

Same comment about default bool.

in src/core_read.cpp:147 in 6e9e4e6130 outdated

141@@ -142,9 +142,9 @@ static bool DecodeTx(CMutableTransaction& tx, const std::vector<unsigned char>&
142     // Try decoding with extended serialization support, and remember if the result successfully
143     // consumes the entire input.
144     if (try_witness) {
145-        CDataStream ssData(tx_data, SER_NETWORK, PROTOCOL_VERSION);
146+        DataStream ssData(tx_data);
147         try {
148-            ssData >> tx_extended;
149+            ssData >> TX_WITH_WITNESS(tx_extended);

theuni commented at 7:36 pm on November 14, 2023:

Nice, this code is much more straightforward now.

in src/net_processing.cpp:2421 in 6e9e4e6130 outdated

2417@@ -2418,8 +2418,8 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
2418         CTransactionRef tx = FindTxForGetData(*tx_relay, ToGenTxid(inv));
2419         if (tx) {
2420             // WTX and WITNESS_TX imply we serialize with witness
2421-            int nSendFlags = (inv.IsMsgTx() ? SERIALIZE_TRANSACTION_NO_WITNESS : 0);
2422-            m_connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *tx));
2423+            const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS);

theuni commented at 7:48 pm on November 14, 2023:

Beautiful :)

in src/net_processing.cpp:4122 in 6e9e4e6130 outdated

4118@@ -4119,7 +4119,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4119             LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because active chain has too little work; sending empty response\n", pfrom.GetId());
4120             // Just respond with an empty headers message, to tell the peer to
4121             // go away but not treat us as unresponsive.
4122-            m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::HEADERS, std::vector<CBlock>()));
4123+            m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::HEADERS, std::vector<CBlockHeader>()));

theuni commented at 7:52 pm on November 14, 2023:

I guess this makes more sense than defining witness-ness for an empty block, but just to clarify…

This is a no-op and would’ve been a no-op independent of this PR as well, right?

ajtowns commented at 1:54 am on November 15, 2023:

Yes – serializing an empty vector is just WriteCompactSize(s, 0) no matter the elements that would have been in the vector, but to serialize a vector of blocks (even an empty one) you need to decide what to do about witness data (even though there is no witness data in an empty block), whereas you don’t need to do that for block headers.

in src/node/blockstorage.cpp:959 in 6e9e4e6130 outdated

955@@ -956,7 +956,7 @@ bool BlockManager::WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const
956     }
957 
958     // Write index header
959-    unsigned int nSize = GetSerializeSize(block, fileout.GetVersion());
960+    unsigned int nSize = GetSerializeSize(TX_WITH_WITNESS(block));

theuni commented at 7:56 pm on November 14, 2023:

If version is no longer used for fileout in these ops, can’t we s/CAutoFile/AutoFile/g ?

ajtowns commented at 3:24 am on November 15, 2023:

Little bit of a rabbit hole to do that – need to change BufferedFile to accept an AutoFile first etc. Doesn’t seem worth adding here to me. See https://github.com/ajtowns/bitcoin/commits/202311-autofile

in src/primitives/transaction.cpp:105 in 6e9e4e6130 outdated

101@@ -102,7 +102,7 @@ CAmount CTransaction::GetValueOut() const
102 
103 unsigned int CTransaction::GetTotalSize() const
104 {
105-    return ::GetSerializeSize(*this, PROTOCOL_VERSION);
106+    return ::GetSerializeSize(TX_WITH_WITNESS(*this));

theuni commented at 7:59 pm on November 14, 2023:

Again, so much more clear. Nice.

I think we could probably drop version.h here now?

Edit: Oooh, we can drop it from some deep internal headers even (hash.h and consensus/validation.h). ~See https://github.com/theuni/bitcoin/commit/9986da90f604209c42dcc65a47455ea23b2f7a5a~ . I think I’d prefer to see that added to this PR so that we don’t put it off.

Edit2: Updated version here as I missed undo.h the first time: https://github.com/theuni/bitcoin/commit/3762e583cdea9f043e50df24e5aa547a9ce3dcec

ajtowns commented at 2:05 am on November 15, 2023:

Might be better to do that after dropping version from GetSerializeSize ? see #28438 (review)

ajtowns commented at 2:43 am on November 15, 2023:

See #28878. Does that seem okay?

in src/rpc/rawtransaction.cpp:1544 in 6e9e4e6130 outdated

1540@@ -1541,7 +1541,7 @@ static RPCHelpMan finalizepsbt()
1541     std::string result_str;
1542 
1543     if (complete && extract) {
1544-        ssTx << mtx;
1545+        ssTx << TX_WITH_WITNESS(mtx);

theuni commented at 8:37 pm on November 14, 2023:

These can be a DataStreams now?

ajtowns commented at 2:38 am on November 15, 2023:

None of the psbt bits can switch to DataStream until CVectorWriter is updated.

in src/rpc/server.cpp:600 in 6e9e4e6130 outdated

600 {
601-    int flag = 0;
602-    if (gArgs.GetIntArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) == 0)
603-        flag |= SERIALIZE_TRANSACTION_NO_WITNESS;
604-    return flag;
605+    return (gArgs.GetIntArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) == 0);

theuni commented at 8:39 pm on November 14, 2023:

I realize this isn’t new, but it’s pretty icky to go to gArgs for something as low-level as serialization. Makes me wonder if we’re doing that in any tight loops. Nothing to do with this PR, but it be nice to cache this in a local bool.

maflcko commented at 8:14 am on November 15, 2023:

This will also go away before the next release, see #28438 (review)

theuni commented at 8:45 pm on November 14, 2023: member

I tried to step through everything to verify the correctness of the witness vs non-witness choices in the changed code. I agree with all of them.

Only skimmed the tests.

I left a comment about cleaning up version.h includes which I believe should be included as part of this PR.

Looks good otherwise.

theuni commented at 9:26 pm on November 14, 2023: member

Btw, the version.h suggesed change above, while large, can be trivially reviewed using git diff --stat, as #include <version.h> is either added or removed in one line and that’s it.

theuni approved

theuni commented at 1:58 pm on November 15, 2023: member

Thanks for addressing my feedback. Sounds like you already have plans for the things I complained about, so no reason to hold this up.

ACK a0c254c13a3ef21e257cca3493446c632b636b15

fanquake merged this on Nov 15, 2023

fanquake closed this on Nov 15, 2023

maflcko commented at 3:31 pm on November 15, 2023: member

@ajtowns To avoid duplicate work, it would be good if you opened all follow-up pulls you wanted to do, or reply with a list here. Others can then take the remaining follow-ups :)

ajtowns commented at 0:42 am on November 16, 2023: contributor

@ajtowns To avoid duplicate work, it would be good if you opened all follow-up pulls you wanted to do, or reply with a list here. Others can then take the remaining follow-ups :)

I think followups are:

Drop version from GetSerializeSize()
- #28878
CVectorWriter to VectorWriter; net updates
- #28892
Drop CAutoFile
- #28904
Drop version from SpanReader, allow psbt’s to be serialized via DataStream
- #28912
Drop SER_*
- #28451
Drop CDataStream; drop GetVersion(), GetType() ?
Drop -rpcserialversion and the without_witness params to TxToUniv and EncodeHexTx
- #28890
Support multiple serparams in a single stream (ipc/multiprocess)
- #28929

EDIT: err, I’m not particularly attached to any of these if anyone wants to take them up

maflcko commented at 2:26 pm on November 16, 2023: member

CVectorWriter to VectorWriter, drop version from SpanReader, allow psbt’s to be serialized via DataStream; Drop CDataStream; drop GetVersion(), GetType() ?

I did VectorWriter as part of #28892, which seems already large enough to be its own pull. The rest can be done in a follow-up, I guess.

Drop -rpcserialversion and the without_witness params to TxToUniv and EncodeHexTx

Done in #28890

CAutoFile

Looks like your commits are based on #https://github.com/bitcoin/bitcoin/pull/28878, so I guess that is the next pull to review for now?

maflcko commented at 11:31 am on November 20, 2023: member

SpanReader

Done in #28912. I guess the CDataStream one can be done as part of #https://github.com/bitcoin/bitcoin/pull/28451

in src/primitives/transaction.h:196 in a0c254c13a

188@@ -197,6 +189,13 @@ class CTxOut
189 
190 struct CMutableTransaction;
191 
192+struct TransactionSerParams {
193+    const bool allow_witness;
194+    SER_PARAMS_OPFUNC
195+};
196+static constexpr TransactionSerParams TX_WITH_WITNESS{.allow_witness = true};

ryanofsky commented at 2:58 pm on November 21, 2023:

I’m wondering if TX_WITH_WITNESS calls should be required and specified everywhere (107 places)

If you write stream << object, it seems like it would make sense for the object to be fully serialized by default, with an option to partially serialize it. I’m asking in the context of #28921 / #10102, because before this PR, CTransaction objects could be serialized normally like other objects, and now they require a TX_WITH_WITNESS wrapper to have the same behavior. If TX_WITH_WITNESS were just the default serialization behavior, I wouldn’t need a special case for CTransaction objects in multiprocess code, and maybe other code could be simplified too. Does this make sense?

ajtowns commented at 0:55 am on November 22, 2023:

The approach we’ve got looks like:

0    dumb_serializer << SER_PARAMS(object)
1    // file, CTransaction

which then gets translated to:

0    dumb_serializer << smart_object
1    // file, ParamsWrapper<TransactionSerParams, CTransaction>

then

0    smart_serializer.Serialize(object, SER_PARAMS)
1    // ParamsStream<TransactionSerParams, dumb_serializer>, CTransaction

The advantage of saying TX_WITH_WITNESS everywhere is that way we’re being explicit about how objects are serialised to dumb streams everywhere, so we get compile errors if we’re ever potentially inconsistent.

But I don’t think that makes sense for multiprocess: we don’t need to be using dumb streams in the first place there because the whole point is to pass objects between our own processes, no? So afaics, we should be using a smart serializer from the start; ie one that provides a GetParams() that returns something that can be be converted to TransactionSerParams. So doing ParamsStream ss{TX_WITH_WITNESS, stream} in CustomBuildField and CustomReadField would be an easy way to to start?

If you want to pass around both CNetAddr and CAddress objects as well, I think this would work:

 0// serialize.h
 1template<typename... SerParams>
 2class MultiSerParam
 3{
 4private:
 5    const std::tuple<SerParams...> m_params;
 6public:
 7    constexpr MultiSerParam(const SerParams&... t) : m_params{t...} { }
 8
 9    template<typename T>
10    constexpr operator const T&() const { return std::get<T>(m_params); }
11};
12
13// ipc/capnp/common-types.h ?
14static constexpr MultiSerParam INTERNAL_SER_PARAMS(
15  TX_WITH_WITNESS,
16  CNetAddr::V2,
17  CAddress::V2_NETWORK
18);
19
20void f(const CNetAddr& netaddr, const CAddress& addr, const CTransaction& tx)
21{
22    DataStream substream;
23    ParamsStream ss{INTERNAL_SER_PARAMS, substream};
24    ss << netaddr;
25    ss << tx;
26    ss << addr;
27}

Note that making this actuall compile also requires tweaking CNetAddr like so:

0-        if (s.GetParams().enc == Encoding::V2) {
1+        CNetAddr::SerParams p(s.GetParams());
2+        if (p.enc == Encoding::V2) {

ryanofsky commented at 9:50 am on November 22, 2023:

Thanks! That approach of being able to combine different params is clever and solves my problem, avoiding boilerplate from having to create different streams for each object type.

Maybe at some point it would be useful to support default parameter values, too, so parameters like CNetAddr::V2 and CAddress::V2_NETWORK could be assumed if not specified. For some serialization parameters it may be better to be explicit, but for others there could be downsides. For example if I hardcode V2 in multiprocess code and a V3 serialization is added later it could cause subtle bugs if V3 data is silently lost in IPC calls. But there could be other solutions to this problem like having IpcSerParams with an option to fully serialize all data. In any case this is just a theoretical problem, so I have a good idea of what to do now.

ajtowns commented at 11:51 am on November 22, 2023:

If the issue is the separation between the CNetAddr::SerParams declaration and the MultiSerParams(..,CNetAddr::V2,..) use, could instead make it:

 0class IPCSerParam
 1{
 2public:
 3    template<typename T>
 4    operator T&() const { return T::GetIPCSerParams(); }
 5};
 6static constexpr IPCSerParam IPC_SER_PARAMS;
 7
 8class CNetAddr {
 9    struct SerParams {
10        const Encoding enc;
11        static SerParams GetIPCSerParams() { return {Encoding::V2}; }
12        SER_PARAMS_OPFUNC
13   };
14};

That leaves it explicit for the dumb streams, and provides a clear, localised default for internal serialization of the object?

(I’m more worried about changing a default from v2 to v3 causing subtle bugs when deserialising network data or from a file, so I prefer being explicit everywhere and finding some other way to fix the other annoyances)

ryanofsky commented at 1:55 am on November 23, 2023:

I guess the idea of having default parameters is off the table then. So probably the way forward to make IPC code safer is have an IPC serialization parameters like your suggestion.

For now though I left the safety issue aside and implemented a way for IPC code to pass multiple stream parameters to ParamsStream in #28929 (used here in #10102), so the same stream can be used for all serialization, and there is no need for code that creates different streams depending on the object type. This solves the problem of parameters increasing boilerplate in multiprocess code.

ajtowns commented at 2:33 am on November 23, 2023:

I’m just saying what my opinion is; doesn’t mean I’m necessarily right. What you’ve come up with looks pretty good to me though.

ryanofsky commented at 4:12 pm on November 28, 2023:

re: #28438 (review)

I think your v2->v3 example provides a convincing reason why defaulting to the latest version is not always safe. Before this, I was thinking that even if default transaction parameters were not a good idea, making the latest versions default would probably be good. But since this isn’t the case, relying on defaults really isn’t a good general solution and IPC code probably should specify some explicit parameters, even if they are not hardcoded version numbers. Another way to do this wouldn’t require adding new IPC parameter type could be to define aliases like static constexpr SerParams MAX_VERSION = V2 and have IPC code use MAX_VERSION.

Use serialization parameters for CTransaction #28438

Code Coverage

Reviews

Conflicts