validation: followups for de-duplication of packages #23804

pull glozow wants to merge 6 commits into bitcoin:master from glozow:2021-12-dedup-packages changing 4 files +292 −24
  1. 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
  2. DrahtBot added the label Validation on Dec 17, 2021
  3. 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?
  4. 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:
  5. t-bast approved
  6. t-bast commented at 6:15 pm on December 17, 2021: member
  7. achow101 commented at 6:30 pm on December 17, 2021: member
    ACK fcc2f91ed48b748c19cb99fad40f57de6cd3a445
  8. fanquake requested review from ariard on Dec 18, 2021
  9. ariard commented at 0:40 am on December 20, 2021: member

    ACK fcc2f91

    Thanks for taking the comments from #22674!

  10. 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
  11. 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.
  12. 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
  13. 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.
  14. 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.
  15. 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.
  16. 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.
  17. glozow force-pushed on Jan 7, 2022
  18. 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.
  19. 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
  20. t-bast approved
  21. [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
  22. [doc] more detailed explanation for deduplication 9ad211c575
  23. [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
  24. AcceptPackage fixups
    No behavior changes, just clarifications.
    9d88853e0c
  25. [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
  26. glozow force-pushed on Jan 17, 2022
  27. 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
  28. [unit test] package parents are a mix 3cd7f693d3
  29. glozow force-pushed on Jan 17, 2022
  30. achow101 commented at 6:16 pm on January 17, 2022: member
    ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972
  31. t-bast approved
  32. fanquake added this to the milestone 23.0 on Jan 18, 2022
  33. fanquake requested review from instagibbs on Jan 18, 2022
  34. instagibbs approved
  35. instagibbs commented at 3:51 am on January 20, 2022: member
    ACK 3cd7f693d3ed1bb7cf9ba3e0c482174df3684972
  36. fanquake requested review from darosior on Jan 20, 2022
  37. glozow requested review from ariard on Jan 21, 2022
  38. 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.
  39. 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.

  40. fanquake merged this on Jan 25, 2022
  41. fanquake closed this on Jan 25, 2022

  42. glozow deleted the branch on Jan 25, 2022
  43. sidhujag referenced this in commit 81e7f6528e on Jan 28, 2022
  44. ClaraBara22 commented at 3:31 pm on March 6, 2022: none
    Can I comment?
  45. Fabcien referenced this in commit 156fb9c3af on Dec 19, 2022
  46. Fabcien referenced this in commit ddd28355ff on Dec 19, 2022
  47. DrahtBot locked this on Mar 6, 2023

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-10-08 07:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me