validation: followups for de-duplication of packages

glozow commented at 4:15 pm on December 17, 2021: member

This addresses some comments from review on e12fafda2dfbbdf63f125e5af797ecfaa6488f66 from #22674.

Improve documentation about de-duplication: comment
Fix code looking up same-txid-different-witness transaction in mempool: comment
Improve the interface for when a same-txid-different-witness transaction is swapped: comment
Add a test for witness swapping: comment
Add a test for packages with a mix of duplicate/different witness/new parents: comment
Fix issue with not notifying CValidationInterface when there’s a partial submission due to fail-fast: comment

DrahtBot added the label Validation on Dec 17, 2021

in src/test/txpackage_tests.cpp:408 in fcc2f91ed4 outdated

403+        auto it_parent2_deduped = submit_witness2.m_tx_results.find(ptx_parent->GetWitnessHash());
404+        auto it_child2 = submit_witness2.m_tx_results.find(ptx_child2->GetWitnessHash());
405+        BOOST_CHECK(it_parent2_deduped != submit_witness2.m_tx_results.end());
406+        BOOST_CHECK(it_parent2_deduped->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
407+        BOOST_CHECK(it_child2 != submit_witness2.m_tx_results.end());
408+        BOOST_CHECK(it_child2->second.m_result_type == MempoolAcceptResult::ResultType::WITNESS_SWAPPED);

t-bast commented at 6:13 pm on December 17, 2021:

Can you check that the wtxid inside WITNESS_SWAPPED is the wtxid of child1?

in src/test/txpackage_tests.cpp:424 in fcc2f91ed4 outdated

415+    // the in-mempool transaction, child1. Since child1 exists in the mempool and its outputs are
416+    // available, child2 should be swapped and granchild should be accepted.
417+    //
418+    // This tests a potential censorship vector in which an attacker broadcasts a competing package
419+    // where a parent's witness is mutated. The honest package should be accepted despite the fact
420+    // that we don't allow witness replacement.

t-bast commented at 6:14 pm on December 17, 2021:

:clap:

t-bast approved

t-bast commented at 6:15 pm on December 17, 2021: member

Tiny nit about the test, but LGTM. ACK https://github.com/bitcoin/bitcoin/pull/23804/commits/fcc2f91ed48b748c19cb99fad40f57de6cd3a445

achow101 commented at 6:30 pm on December 17, 2021: member

ACK fcc2f91ed48b748c19cb99fad40f57de6cd3a445

fanquake requested review from ariard on Dec 18, 2021

ariard commented at 0:40 am on December 20, 2021: member

ACK fcc2f91

Thanks for taking the comments from #22674!

in src/validation.cpp:1267 in fcc2f91ed4 outdated

1254@@ -1255,10 +1255,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
1255 
1256     LOCK(m_pool.cs);
1257     std::map<const uint256, const MempoolAcceptResult> results;
1258-    // As node operators are free to set their mempool policies however they please, it's possible
1259-    // for package transaction(s) to already be in the mempool, and we don't want to reject the
1260-    // entire package in that case (as that could be a censorship vector).  Filter the transactions
1261-    // that are already in mempool and add their information to results, since we already have them.
1262+    // Node operators are free to set their mempool policies however they please, nodes may receive

jnewbery commented at 10:47 am on January 1, 2022:

Referring to node operators setting mempool policies differently is just confusing. This can happen even if every node on the network has the same policy - it’s just a property of the p2p network that transactions can arrive in different orders.

glozow commented at 11:31 am on January 7, 2022:

transactions arriving in different orders is also listed as a possible reason

in src/test/txpackage_tests.cpp:426 in fcc2f91ed4 outdated

419+    // where a parent's witness is mutated. The honest package should be accepted despite the fact
420+    // that we don't allow witness replacement.
421+    CKey grandchild_key;
422+    grandchild_key.MakeNewKey(true);
423+    CScript grandchild_locking_script = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey()));
424+    auto mtx_grandchild = CreateValidMempoolTransaction(/* input_transaction */ ptx_child2, /* vout */ 0,

jnewbery commented at 10:56 am on January 1, 2022:

Use comments that clang-tidy can understand:

0    auto mtx_grandchild = CreateValidMempoolTransaction(/*input_transaction=*/ptx_child2, /*vout=*/0,

etc. There are a few other places that can also be updated.

(see #23545)

glozow commented at 12:40 pm on January 17, 2022:

Done, applied to all the test code that’s added in this PR.

in src/test/txpackage_tests.cpp:337 in fcc2f91ed4 outdated

332+// the mempool.
333+BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
334+{
335+    LOCK(cs_main);
336+
337+    // Transactions with a same-txid-different-witness transaction in the mempool should be swapped.

jnewbery commented at 11:04 am on January 1, 2022:

I think the word swapped is slightly confusing here. It suggests to me that child1 will be replaced (“swapped”) with child2 in the mempool.

glozow commented at 5:05 pm on January 7, 2022:

Changed result enum to DIFFERENT_WITNESS and verb from “swapped” to “ignored” in comments

in src/test/txpackage_tests.cpp:416 in fcc2f91ed4 outdated

411+        BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_child2->GetWitnessHash())));
412+    }
413+
414+    // Try submitting Package1{child2, grandchild} where child2 is same-txid-different-witness as
415+    // the in-mempool transaction, child1. Since child1 exists in the mempool and its outputs are
416+    // available, child2 should be swapped and granchild should be accepted.

jnewbery commented at 11:04 am on January 1, 2022:

0    // available, child2 should be swapped and grandchild should be accepted.

jnewbery commented at 11:04 am on January 1, 2022:

I also think the word “swapped” here is confusing.

in src/test/txpackage_tests.cpp:360 in fcc2f91ed4 outdated

352+    witness2.stack.push_back(std::vector<unsigned char>(2));
353+    witness2.stack.push_back(std::vector<unsigned char>(witnessScript.begin(), witnessScript.end()));
354+
355+    CKey child_key;
356+    child_key.MakeNewKey(true);
357+    CScript child_locking_script = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey()));

jnewbery commented at 11:21 am on January 1, 2022:

I think this is slightly confusing terminology, but I realise it’s the same as the other tests in this file, so maybe should be left as it is for now. The child transaction’s output is locked with a public key, which is signed for with the corresponding private key in the grandchild transaction. I therefore think it’d be clearer if this was named grandchild_key, since the entity that creates the grandchild transaction owns this private key.

in src/validation.h:165 in fcc2f91ed4 outdated

161@@ -162,8 +162,13 @@ struct MempoolAcceptResult {
162         VALID, //!> Fully validated, valid.
163         INVALID, //!> Invalid.
164         MEMPOOL_ENTRY, //!> Valid, transaction was already in the mempool.
165+        WITNESS_SWAPPED, //!> A same-txid-different-witness tx already exists in the mempool, and was not replaced.

jnewbery commented at 11:30 am on January 1, 2022:

Again, I think this terminology is a bit misleading, since swapped suggests the witness was replaced in the mempool.

jnewbery commented at 11:31 am on January 1, 2022: member

I’ve left a few minor comments here, but there are a couple of more substantial comments in the original PR here: #22674#pullrequestreview-837111576.

glozow force-pushed on Jan 7, 2022

glozow commented at 5:53 pm on January 7, 2022: member

Addressed comments and added the fixups from @jnewbery’s review on #22674 - most have no behavior changes so they’re grouped in 1 commit. Another commit fixes the issue where we’re not notifying MainSignals when there’s a partial submission.

in src/validation.h:169 in 1098f39db3 outdated

160@@ -161,8 +161,13 @@ struct MempoolAcceptResult {
161         VALID, //!> Fully validated, valid.
162         INVALID, //!> Invalid.
163         MEMPOOL_ENTRY, //!> Valid, transaction was already in the mempool.
164+        DIFFERENT_WITNESS, //!> Not validated. A same-txid-different-witness tx (see m_other_wtxid) already exists in the mempool and was not replaced.
165     };
166+    /** Result type. Present in all MempoolAcceptResults. */
167     const ResultType m_result_type;
168+
169+    // The following field is only present when m_result_type = ResultType::INVALID

instagibbs commented at 2:35 am on January 10, 2022:

field seems to exist in either case?

glozow commented at 3:28 pm on January 11, 2022:

Ah yeah, not nonexistent, but unset. Default is valid, with all fields empty, so not much meaning.

glozow commented at 12:40 pm on January 17, 2022:

Removed this comment

t-bast approved

t-bast commented at 8:15 am on January 10, 2022: member

ACK https://github.com/bitcoin/bitcoin/pull/23804/commits/0300cb10094efcca1832acaa189e365b7bedaec3

[packages] return DIFFERENT_WITNESS for same-txid-different-witness tx

The previous interface required callers to guess that the tx had been
swapped and look up the tx again by txid to find a `MEMPOOL_ENTRY`
result. This is a confusing interface.

Instead, explicitly tell the caller that this transaction was
`DIFFERENT_WITNESS` in the result linked to the mempool entry's wtxid.
This gives the caller all the information they need in 1 lookup, and
they can query the mempool for the other transaction if needed.

83d4fb7126

[doc] more detailed explanation for deduplication 9ad211c575

[unit test] different witness in package submission

If/when witness replacement is implemented in the future, this test case
can be easily replaced with witness replacement tests.

2db77cd3b8

AcceptPackage fixups

No behavior changes, just clarifications.

9d88853e0c

[validation] better handle errors in SubmitPackage

Behavior change: don't quit right after LimitMempoolSize() when a
package is partially submitted. We should still send
TransactionAddedToMempool notifications for
transactions that were submitted.

Not behavior change: add a new package validation result for mempool logic errors.

de075a98ea

glozow force-pushed on Jan 17, 2022

glozow commented at 12:44 pm on January 17, 2022: member

Added more tests for funsies, the rest of the diff is just changing comments. @ariard @achow101 @jnewbery @instagibbs @t-bast if you have time to take another look? Would be nice to add these fixups with #22674 before v23

[unit test] package parents are a mix 3cd7f693d3

glozow force-pushed on Jan 17, 2022

achow101 commented at 6:16 pm on January 17, 2022: member

ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972

t-bast approved

t-bast commented at 6:46 pm on January 17, 2022: member

LGTM, ACK https://github.com/bitcoin/bitcoin/pull/23804/commits/3cd7f693d3ed1bb7cf9ba3e0c482174df3684972

fanquake added this to the milestone 23.0 on Jan 18, 2022

fanquake requested review from instagibbs on Jan 18, 2022

instagibbs approved

instagibbs commented at 3:51 am on January 20, 2022: member

ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972

fanquake requested review from darosior on Jan 20, 2022

glozow requested review from ariard on Jan 21, 2022

in src/validation.cpp:1091 in de075a98ea outdated

1095@@ -1088,7 +1096,6 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
1096     LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(),
1097                      gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000,
1098                      std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)});
1099-    if (!all_submitted) return false;

ariard commented at 1:56 am on January 25, 2022:

Well technically if this code change now trigger a TransactionAddedToMempool(), I think we’re changing the views of our CMainSignals consumers and that would constitute a behavior change. That said, if I’m correct, I don’t think it’s worthy to invalidate ACKs just to update commit description.

ariard commented at 1:59 am on January 25, 2022: member

ACK 3cd7f69

Substantial changes since my last ACK are new commits adding clarifications, improving the errors handling and extending the unit test.

fanquake merged this on Jan 25, 2022

fanquake closed this on Jan 25, 2022

glozow deleted the branch on Jan 25, 2022

sidhujag referenced this in commit 81e7f6528e on Jan 28, 2022

ClaraBara22 commented at 3:31 pm on March 6, 2022: none

Can I comment?

Fabcien referenced this in commit 156fb9c3af on Dec 19, 2022

Fabcien referenced this in commit ddd28355ff on Dec 19, 2022

DrahtBot locked this on Mar 6, 2023

validation: followups for de-duplication of packages #23804