Use serialization parameters for CTransaction #28438
pull ajtowns wants to merge 3 commits into bitcoin:master from ajtowns:202309-txwithparams changing 62 files +228 −277-
ajtowns commented at 4:41 am on September 9, 2023: contributor
-
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.
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 force-pushed on Sep 9, 2023DrahtBot added the label CI failed on Sep 9, 2023DrahtBot added the label Needs rebase on Sep 9, 2023ajtowns force-pushed on Sep 9, 2023DrahtBot removed the label Needs rebase on Sep 9, 2023DrahtBot removed the label CI failed on Sep 10, 2023sipa commented at 10:17 pm on September 10, 2023: memberIt 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: contributorIt 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, 2023maflcko commented at 7:40 am on September 11, 2023: member- This is mainly because that uses
RPCSerializationFlags()
andrpcSerializationFlags()
interfaces which don’t really match the new model.
I don’t understand this. It should be trivial to return a
TransactionSerParams
fromRPCSerializationFlags
, 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:
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 styleajtowns commented at 8:27 am on September 11, 2023: contributorI don’t understand this. It should be trivial to return a
TransactionSerParams
fromRPCSerializationFlags
, 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
andDataStream
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 withDataStream
?
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, 2023maflcko commented at 12:04 pm on September 13, 2023: memberWould 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: contributorWould 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, 2023DrahtBot removed the label Needs rebase on Sep 21, 2023DrahtBot added the label CI failed on Sep 24, 2023ajtowns force-pushed on Sep 26, 2023sipa commented at 2:24 pm on September 26, 2023: memberConcept/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, 2023ajtowns force-pushed on Oct 2, 2023DrahtBot removed the label Needs rebase on Oct 2, 2023DrahtBot removed the label CI failed on Oct 2, 2023DrahtBot added the label Needs rebase on Oct 16, 2023ajtowns force-pushed on Oct 17, 2023DrahtBot added the label CI failed on Oct 17, 2023DrahtBot removed the label Needs rebase on Oct 17, 2023DrahtBot removed the label CI failed on Oct 19, 2023DrahtBot added the label Needs rebase on Oct 26, 2023ajtowns force-pushed on Oct 27, 2023ajtowns marked this as ready for review on Oct 27, 2023DrahtBot removed the label Needs rebase on Oct 27, 2023DrahtBot added the label CI failed on Nov 1, 2023ajtowns force-pushed on Nov 1, 2023in 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 thatSERIALIZE_TRANSACTION_NO_WITNESS
was never embedded into theCDataStream
in each context.
maflcko commented at 10:18 am on November 1, 2023:But then you are changing it toDataStream
insrc/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 thatSERIALIZE_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 fromCTransaction
)fanquake requested review from TheCharlatan on Nov 1, 2023in 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 theTransactionCompression
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 havePushMessage(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 dropTransactionCompression
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 haveBIP_XXX_TX_COMPRESSION
be aBlockTransactionsSerParams
and have the logic for compression-or-not happen inBlockTransactions::Serialize
, or it could be a separate formatter. In either case theTX_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, 2023in 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 removePROTOCOL_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 approvedmaflcko commented at 11:14 am on November 1, 2023: memberlgtm, 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, 2023in 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 impliesinline
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, 2023maflcko commented at 1:34 pm on November 1, 2023: memberre-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, 2023maflcko commented at 1:45 pm on November 1, 2023: memberre-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, 2023DrahtBot added the label Needs rebase on Nov 8, 2023ajtowns force-pushed on Nov 8, 2023DrahtBot removed the label Needs rebase on Nov 8, 2023DrahtBot added the label CI failed on Nov 8, 2023DrahtBot removed the label CI failed on Nov 9, 2023maflcko commented at 3:44 pm on November 9, 2023: memberre-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, 2023Use ParamsWrapper for witness serialization 6e9e4e6130Drop OverrideStream c94f7e5b1cDrop CHashWriter a0c254c13aajtowns force-pushed on Nov 13, 2023DrahtBot removed the label Needs rebase on Nov 13, 2023maflcko commented at 2:01 pm on November 14, 2023: memberre-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.
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 justWriteCompactSize(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 wes/CAutoFile/AutoFile/g
?
ajtowns commented at 3:24 am on November 15, 2023:Little bit of a rabbit hole to do that – need to changeBufferedFile
to accept anAutoFile
first etc. Doesn’t seem worth adding here to me. See https://github.com/ajtowns/bitcoin/commits/202311-autofilein 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
andconsensus/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 fromGetSerializeSize
? see #28438 (review)
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 aDataStream
s now?
ajtowns commented at 2:38 am on November 15, 2023:None of the psbt bits can switch toDataStream
untilCVectorWriter
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 togArgs
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: memberI 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: memberBtw, theversion.h
suggesed change above, while large, can be trivially reviewed usinggit diff --stat
, as#include <version.h>
is either added or removed in one line and that’s it.theuni approvedtheuni commented at 1:58 pm on November 15, 2023: memberThanks 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, 2023fanquake closed this on Nov 15, 2023
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()
CVectorWriter
toVectorWriter
; net updates- Drop
CAutoFile
- Drop version from
SpanReader
, allow psbt’s to be serialized viaDataStream
- Drop
SER_*
- Drop
CDataStream
; dropGetVersion(), GetType()
? - Drop
-rpcserialversion
and thewithout_witness
params toTxToUniv
andEncodeHexTx
- Support multiple serparams in a single stream (ipc/multiprocess)
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: memberCVectorWriter
toVectorWriter
, drop version fromSpanReader
, allow psbt’s to be serialized viaDataStream
; 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?
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 toTransactionSerParams
. So doingParamsStream ss{TX_WITH_WITNESS, stream}
inCustomBuildField
andCustomReadField
would be an easy way to to start?If you want to pass around both
CNetAddr
andCAddress
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 theMultiSerParams(..,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 useMAX_VERSION
.bitcoin locked this on Nov 27, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me