mempool: delete exists(uint256) function by glozow · Pull Request #23325 · bitcoin/bitcoin

glozow · 2021-10-20T15:59:30Z

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:

bool 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. 🐛

Another approach would be to try both:

bool 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.

src/net_processing.cpp

JeremyRubin · 2021-10-20T17:05:32Z

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

struct Wtxid {
    uint256 h;
   explicit Wtxid(uint256 i): h(i);
}

struct Txid {
    uint256 h;
   explicit Txid(uint256 i): h(i);
}

class CTxMempool{
    bool exists(Txid t);
    bool exists(Wtxid t);
}

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

DrahtBot · 2021-10-21T05:28:38Z

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.

glozow · 2021-10-21T08:29:47Z

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.

maflcko · 2021-10-21T08:34:37Z

I am not sure if it is possible to make Wtxid and Txid different types. Wouldn't that require a major rewrite of txrequest?

glozow · 2021-10-21T08:40:30Z

True, we'd need to change the Announcement struct to store a GenTxid instead of uint256 and bool, which would probably increase the size...

naumenkogs · 2021-10-21T11:57:29Z

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.

jnewbery · 2021-10-21T13:38:55Z

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.

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.

glozow · 2021-10-21T15:36:14Z

Thanks for the reviews! I've added the explicit ctors.

JeremyRubin · 2021-10-21T15:43:01Z

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.

laanwj · 2021-10-21T16:51:22Z

src/primitives/transaction.h

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


Would be nice to make this constructor private / protected at some point (not here though)

laanwj · 2021-10-21T16:52:01Z

Code review ACK 4307849

jnewbery · 2021-10-21T16:56:46Z

Tested and code review ACK 4307849

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?

maflcko · 2021-10-22T09:53:29Z

review ACK 4307849 👘

Show signature and timestamp

Signature:

maflcko reviewed Oct 20, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot added Mempool P2P RPC/REST/ZMQ TX fees and policy Validation labels Oct 20, 2021

maflcko added Refactoring Mempool and removed TX fees and policy RPC/REST/ZMQ P2P Validation Mempool labels Oct 20, 2021

DrahtBot mentioned this pull request Oct 21, 2021

[policy] check ancestor feerate in RBF, remove BIP125 Rule2 #23121

Closed

glozow added 2 commits October 21, 2021 16:26

create explicit GenTxid::{Txid, Wtxid} ctors

d50fbd4

[mempool] delete exists(uint256) function

4307849

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.

glozow force-pushed the 2021-10-exists-txid branch from d767a93 to 4307849 Compare October 21, 2021 15:29

laanwj reviewed Oct 21, 2021

View reviewed changes

DrahtBot mentioned this pull request Oct 22, 2021

fix CTxMemPool::TrimToSize to put only confirmed coins in pvNoSpendsRemaining #19880

Closed

maflcko merged commit c001da3 into bitcoin:master Oct 22, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 22, 2021

Merge bitcoin#23325: mempool: delete exists(uint256) function

1fa207a

bitcoin locked and limited conversation to collaborators Oct 30, 2022

glozow deleted the 2021-10-exists-txid branch July 20, 2023 12:12

Conversation

glozow commented Oct 20, 2021

Uh oh!

Uh oh!

JeremyRubin commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 21, 2021

Conflicts

Uh oh!

glozow commented Oct 21, 2021

Uh oh!

maflcko commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Oct 21, 2021

Uh oh!

naumenkogs commented Oct 21, 2021

Uh oh!

jnewbery commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Oct 21, 2021

Uh oh!

JeremyRubin commented Oct 21, 2021

Uh oh!

laanwj Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 21, 2021

Uh oh!

jnewbery commented Oct 21, 2021

Uh oh!

maflcko commented Oct 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JeremyRubin commented Oct 20, 2021 •

edited

Loading

maflcko commented Oct 21, 2021 •

edited

Loading

jnewbery commented Oct 21, 2021 •

edited

Loading