Make nVersion=3 (which is currently nonstandard on mainnet) standard.
Note that we will treat these transactions as Topologically Restricted Until Confirmation (TRUC). Spec is in BIP 431 and implementation is in #28948, #29306, and #29873
See #27463 for overall project tracking, and #29319 for information about relevance to cluster mempool.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | instagibbs, murchandamus, ismaelsadeeq, sdaftuar, achow101 |
| Concept ACK | hernanmarino |
If your review is incorrectly listed, please react with đź‘Ž to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
What is your thinking on #29454 ?
I would still favor to make the 1000 vb limit dynamic and applying on the whole package, not only the child.
Doing with v3 itself would present a) a policy signaling mechanism saving and b) avoid some policy bargains like we had with mempoolfullrbf (between LN-vs-0-confs).
If you prefer to have the discussion on the v3 BIP PR let me know.
What is your thinking on #29454 ?
Same as said that for sibling evictions - I’ll go to hack a tx-relay jamming / pinning toolkit, that way we’ll have more information on the robustness of those current proposed anti-pinning mitigations. I still maintain there are very incomplete for time-sensitive transaction flows, especially in face of “loophole” and NTA pinnings. I’m incredibly worried for the security of the Lightning Network if we don’t deploy additional anti-pinning mitigations on top of those ones over the future years - We have not seen yet script kiddies attacking the network.
Under today inertia, we’ll probably converge towards a centralized network of ~50 LSPs relying on traditional social trust rather than relying on trust-minimized confirmation of pre-signed transactions. This is very problematic if they wish to accept channel traffic from low-resources spokes LN nodes, especially in geographical areas where social trust mechanism cannot be afforded by end-users.
On #29454, I still prefer to keep experimenting on “transaction-issuer selected policy limits” which might encompass more stringent tx-relay jamming on a minor fork of Bitcoin Core. Coming up with a sound technical proposal in matters of tx-relay policy likely demands few rounds of iterations and realistically we already have a severe shortage of qualified review bandwidth in those areas.
The initial reason closing of #29448 was my own realization that the introduction of differentiated tx-relay policy among the tx-relay network of nodes will probably generate with time tiers of transaction economic traffic priority (I really cannot see how it worsens memory / CPU DoS robustness per se ?). I don’t think this is a phenomena we can do a lot as a full-node implementation, as we’re seeing with the appearance of transaction accelerators by mining pools. There is transaction issuer demand for such services apparently. Yet we should be careful of doing works in parallel to maintain bip152 block propagation and as such long-term decentralization of the base-layer network.
I’m not opposing the deployment of TX_MAX_STANDARD_VERSION=3 in Bitcoin Core v28.0.
Rebased
Concept ACK and code changes look simple enough. Given that the proposed rules for V3 transactions entail a voluntary topology restriction by the sender on their own transactions, it seems reasonable to me to merge this into the master branch soon
Concept ACK 7b18f0ed94557a93c69b4f19d85c4fd6fd480464
Added constant for #29873 (review)
Also added a test that wallet does not signal v3 as it uses the default CURRENT_VERSION (this is correct as it wouldn't be safe to make transactions v3 before any of the network has adopted this change). I believe the nVersion can be changed via CCoinControl::m_version, though I'm not sure if that is accessible so I didn't write a test for it.
14 | @@ -15,8 +15,9 @@ 15 | #include <set> 16 | #include <string> 17 | 18 | -// This module enforces rules for transactions with nVersion=3 ("v3 transactions") which help make 19 | +// This module enforces rules for transactions BIP 431 TRUC transactions (with nVersion=3) which help make
There is either too many or too few words here:
// This module enforces rules for BIP 431 TRUC transactions (with nVersion=3) which help make
fixed, thanks
123 | + 124 | + # While v3 transactions are standard, the CURRENT_VERSION is 2. 125 | + # This test can be removed if CURRENT_VERSION is changed, and replaced with tests that the 126 | + # wallet handles v3 rules properly. 127 | + assert_equal(tx_current_version.nVersion, 2) 128 | + wallet_v3.unloadwallet()
Good idea to add this!
Still looks good to me
crACK e41dae322d435cd8b32daf73883b466f30349584
1016 | @@ -1017,7 +1017,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) 1017 | .descendant_count = maybe_rbf_limits.descendant_count + 1, 1018 | .descendant_size_vbytes = maybe_rbf_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT, 1019 | }; 1020 | - if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || ws.m_ptx->nVersion == 3) {
few remaining spots that I can see:
src/test/fuzz/package_eval.cpp:176: tx_mut.nVersion = fuzzed_data_provider.ConsumeBool() ? 3 : CTransaction::CURRENT_VERSION;
src/test/fuzz/tx_pool.cpp:229: tx_mut.nVersion = fuzzed_data_provider.ConsumeBool() ? 3 : CTransaction::CURRENT_VERSION;
src/test/txvalidation_tests.cpp:289: mtx_many_sigops.nVersion = 3;
src/test/util/txmempool.cpp:121: if (tx_info.tx->nVersion == 3) {
src/test/util/txmempool.cpp:136: Assert(parents.begin()->get().GetSharedTx()->nVersion == 3);
src/test/util/txmempool.cpp:141: Assert(parent.get().GetSharedTx()->nVersion != 3);
can also use this constant
thanks! fixed these now
Note that, as CURRENT_VERSION = 2, the wallet will not make transactions
with nVersion=3 yet.
utACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c
14 | @@ -15,8 +15,9 @@ 15 | #include <set> 16 | #include <string> 17 | 18 | -// This module enforces rules for transactions with nVersion=3 ("v3 transactions") which help make 19 | +// This module enforces rules for BIP 431 TRUC transactions (with nVersion=3) which help make
nit, I think the mention of TRUC transactions suffices, from the BIP specification we know they have nVersion=3?
// This module enforces rules for BIP 431 TRUC transactions which help make
The file name could also be renamed to refer to TRUC transactions instead of v3_policy.
145 | @@ -146,14 +146,14 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
146 | } else {
147 | // Non-v3 transactions cannot have v3 parents.
Should we replace all v3 mentions with TRUC for clarity and uniformity. The BIP specification refers to this policy rules as TRUC Transactions.
Sure, can be in another PR
Concept ACK
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c 🛰️
utACK 30a01134c
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c
