mempool: delete exists(uint256) function #23325

1. 
   glozow commented at 3:59 pm on October 20, 2021: member

   We use the same type for txids and wtxids, uint256. In places where we want the ability to pass either one, we distinguish them using GenTxid.

   The (overloaded) CTxMemPool::exists() function is defined as follows:

   0bool exists(const uint256& txid) const { return exists(GenTxid{false, txid}); }
   

   It always assumes that a uint256 is a txid, which is a footgunny interface. Querying by wtxid returns a false negative if the transaction has a witness. :bug:

   Another approach would be to try both:

   0bool exists(const uint256& txid) const { return exists(GenTxid{false, txid}) || exists(GenTxid{false, txid}); }
   

   But that’s slower and wrongfully placing the burden on the callee; the caller always knows whether the hash is a txid or a wtxid.

2. in src/net_processing.cpp:3250 in d767a93fb6 outdated

   3246@@ -3247,7 +3247,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
   3247                 // Always relay transactions received from peers with forcerelay
   3248                 // permission, even if they were already in the mempool, allowing
   3249                 // the node to function as a gateway for nodes hidden behind it.
   3250-                if (!m_mempool.exists(tx.GetHash())) {
   3251+                if (!m_mempool.exists(GenTxid(false, tx.GetHash()))) {
   

   ---

   ---

   

   MarcoFalke commented at 4:32 pm on October 20, 2021:

   GenTxid(false,...) is quite verbose, but still ambiguous. A named argument would be even more verbose. What about a GenTxid constructor that is named Txid/Wtxid or so?

   ---

   

   glozow commented at 4:50 pm on October 20, 2021:

   ooh i like.

   I’d do static ctors with s/GenTxid(false, hash)/GenTxid::Txid(hash) and s/GenTxid(true, hash)/GenTxid::Wtxid(hash)

   ---

   

   jnewbery commented at 1:30 pm on October 21, 2021:

   You could also consider adding two public methods to CTransaction:

   0     const uint256& GetHash() const { return hash; }
   1+    GenTxid GetTxid() const {return GenTxid{false, hash};};
   2     const uint256& GetWitnessHash() const { return m_witness_hash; };
   3+    GenTxid GetWtxid() const {return GenTxid{true, m_witness_hash};};
   

   The call sites would then look something like:

   0-        } else if (m_mempool->exists(GenTxid(false, (*it)->GetHash()))) {
   1+        } else if (m_mempool->exists((*it)->GetTxid())) {
   

   ---

   

   glozow commented at 3:31 pm on October 21, 2021:

   I ended up doing GenTxid::Txid and GenTxid::Wtxid. Some of the call sites are using prevout.hash or just hash rather than grabbing from a transaction, so I think this is simpler.
3. 
   JeremyRubin commented at 5:05 pm on October 20, 2021: contributor

   Why not do a ’newtype’ wrapper and then multiple dispatch?

    0struct Wtxid {
    1    uint256 h;
    2   explicit Wtxid(uint256 i): h(i);
    3}
    4
    5struct Txid {
    6    uint256 h;
    7   explicit Txid(uint256 i): h(i);
    8}
    9
   10class CTxMempool{
   11    bool exists(Txid t);
   12    bool exists(Wtxid t);
   13}
   

   (for a more thorough treatment of newtypes, see https://www.boost.org/doc/libs/1_46_1/boost/strong_typedef.hpp)

4. DrahtBot added the label Mempool on Oct 20, 2021
5. DrahtBot added the label P2P on Oct 20, 2021
6. DrahtBot added the label RPC/REST/ZMQ on Oct 20, 2021
7. DrahtBot added the label TX fees and policy on Oct 20, 2021
8. DrahtBot added the label Validation on Oct 20, 2021
9. MarcoFalke removed the label TX fees and policy on Oct 20, 2021
10. MarcoFalke removed the label RPC/REST/ZMQ on Oct 20, 2021
11. MarcoFalke removed the label P2P on Oct 20, 2021
12. MarcoFalke removed the label Validation on Oct 20, 2021
13. MarcoFalke removed the label Mempool on Oct 20, 2021
14. MarcoFalke added the label Refactoring on Oct 20, 2021
15. MarcoFalke added the label Mempool on Oct 20, 2021
16. 
    DrahtBot commented at 5:28 am on October 21, 2021: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #23121 ([policy] check ancestor feerate in RBF, remove BIP125 Rule2 by glozow)
    • #19880 (fix CTxMemPool::TrimToSize to put only confirmed coins in pvNoSpendsRemaining by markblundeberg)

    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.

17. 
    glozow commented at 8:29 am on October 21, 2021: member

    Right, ideally we have different types for txid and wtxid. I also think we should make GenTxid a std::variant<Txid, Wtxid> instead of overloading everything. @MarcoFalke @JeremyRubin would you prefer I change this PR to add new types, or open a new one? Either way we’d delete this function.
18. 
    MarcoFalke commented at 8:34 am on October 21, 2021: member

    I am not sure if it is possible to make Wtxid and Txid different types. Wouldn’t that require a major rewrite of txrequest?
19. 
    glozow commented at 8:40 am on October 21, 2021: member

    True, we’d need to change the Announcement struct to store a GenTxid instead of uint256 and bool, which would probably increase the size…
20. 
    naumenkogs commented at 11:57 am on October 21, 2021: member

    Concept ACK. I think the confusion is worth addressing. No idea what’s the best way to achieve it though, but I’d avoid a lot of code changes for this matter.
21. 
    jnewbery commented at 1:38 pm on October 21, 2021: member

    Concept ACK. Mixing up txids and wtxids has been a source of several bugs. Anything that we can do to stop just passing uint256s around and instead being explicit about what is expected in function interfaces is an improvement in my opinion.

    I agree that ideally we’d have separate types for Txid and Wtxid (and BlockHash) so the compiler can catch bugs for us, but that seems like a much wider scope than this PR is proposing to improve, so could be left for later.

    I initially thought that this change carried a tiny performance penalty, since exists(const uint256& txid) used to be able to take a reference, and now we’re making a copy of the uint256. However, that exists(const uint256& txid) function was actually making a copy internally anyway, so there’s no change in behaviour here.

22. create explicit GenTxid::{Txid, Wtxid} ctors d50fbd4c5b
23. [mempool] delete exists(uint256) function

    Allowing callers to pass in a uint256 (which could be txid or wtxid)
    but then always assuming that it's a txid is a footgunny interface.

    4307849256
24. glozow force-pushed on Oct 21, 2021
25. 
    glozow commented at 3:36 pm on October 21, 2021: member

    Thanks for the reviews! I’ve added the explicit ctors.
26. 
    JeremyRubin commented at 3:43 pm on October 21, 2021: contributor

    i think the newtype stuff is obviously better, but that can be handled as a separate PR if desired.

    Given the named CTor approach, it’s also possible to change those into static functions returning the newtype wrappers with minimal diff.

27. in src/primitives/transaction.h:395 in 4307849256

    392@@ -393,6 +393,8 @@ class GenTxid
    393     uint256 m_hash;
    394 public:
    395     GenTxid(bool is_wtxid, const uint256& hash) : m_is_wtxid(is_wtxid), m_hash(hash) {}
    

    ---

    ---

    

    laanwj commented at 4:51 pm on October 21, 2021:

    Would be nice to make this constructor private / protected at some point (not here though)
28. 
    laanwj commented at 4:52 pm on October 21, 2021: member

    Code review ACK 4307849256761fe2440d82bbec892d0e8e6b4dd4
29. 
    jnewbery commented at 4:56 pm on October 21, 2021: member

    Tested and code review ACK 4307849256761fe2440d82bbec892d0e8e6b4dd4

    The static ctors seem like a strictly superior way to construct the GenTxid objects. What do you think about a follow-up that removes the default ctor?

30. 
    MarcoFalke commented at 9:53 am on October 22, 2021: member

    review ACK 4307849256761fe2440d82bbec892d0e8e6b4dd4 👘

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3review ACK 4307849256761fe2440d82bbec892d0e8e6b4dd4 👘
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUgDtAwApnU2595pO10vRTuqKZCscCE0DH2Qi/P7/v3ebPKBO+luHWXU0V+4PtaM
     8VdMCfmRJOmrKY9AKEVodNcA6a6kuCdO2cLvunHiWdvAzomYvGd2n7AxWEda3p+wh
     9u7raNPMhF57YSVyKKpaF4MxO46KR6MvJLsNZ3304yMTcAl4gT4ZujnVG4HZOurcW
    10xTIIvYrYlGTssVLJmuOBoUSyJHg4DdcDa8rZfncfDbunkVWT4BzB4PZNB4jGfccG
    119vgUd34kSYajNNeThDa+cJ4TROiigSFhVMMyua8gqd0l8Pm9Kkzjs3YezL5islNQ
    129OH7I+dB/X04xkAIba+Ifs+UjVFRK6tWWZRQ342MvVxs/V1R5S8vW4wKZwpU4TEV
    13D8RST1gLp7REB4KQUADnft9EEyYK8c8yDr+LsxLQd/UKdKgkEQMLVxOeTrhemVx6
    147dov5TofVCI2XJT0azFbZWNSvhHVTbulUE3GrRKBEQLus9XnsMDIrKjoiBINcFRp
    15euiR0wMg
    16=dZ5k
    17-----END PGP SIGNATURE-----
    

31. MarcoFalke merged this on Oct 22, 2021
32. MarcoFalke closed this on Oct 22, 2021

    ---

33. sidhujag referenced this in commit 1fa207a19b on Oct 22, 2021
34. bitcoin locked this on Oct 30, 2022
35. glozow deleted the branch on Jul 20, 2023

Contributors
glozow MarcoFalke jnewbery JeremyRubin DrahtBot naumenkogs laanwj

Labels
Refactoring Mempool