validation: distinguish between same tx and same-nonwitness-data tx in mempool

glozow commented at 2:24 pm on June 15, 2021: member

On master, if you submit a transaction with the same txid but different witness to the mempool, it thinks the transactions are the same. Users submitting through BroadcastTransaction() (i.e. sendrawtransaction or the wallet) don’t get notified that there’s a different transaction in the mempool, although it doesn’t crash. Users submitting through testmempoolaccept() will get a “txn-already-in-mempool” error.

This PR simply distinguishes between txn-already-in-mempool and txn-same-nonwitness-data-in-mempool, without handling them differently: sendrawtransaction still will not throw, but testmempoolaccept will give you a different error.

I believe the intention of #19645 is to allow full swaps of transactions that have different witnesses but identical nonwitness data. Returning a different error message + adding a test was suggested: #19645 (comment) so this is that PR.

glozow renamed this:
~~[validation/mempool] distinguish between same tx and same nonwitness tx already in mempool~~
validation: distinguish between same tx and same-nonwitness-data tx in mempool
on Jun 15, 2021

glozow added the label RPC/REST/ZMQ on Jun 15, 2021

glozow added the label Validation on Jun 15, 2021

in src/validation.cpp:619 in 012341f779 outdated

616+    if (m_pool.exists(GenTxid(true, tx.GetWitnessHash()))) {
617+        // Exact transaction already exists in the mempool.
618         return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool");
619+    } else if (m_pool.exists(GenTxid(false, tx.GetHash()))) {
620+        // Transaction with the same nonwitness data but different witnes (same txid, different
621+        // wtxid) already exists in the mempool. TODO: allow replacements

jnewbery commented at 4:02 pm on June 15, 2021:

TODOs in comments are generally discouraged. If someone wants to implement witness replacement, they don’t need a TODO to tell them to do that.

glozow commented at 8:02 am on June 16, 2021:

Gotcha, fixed.

in test/functional/mempool_wtxid.py:48 in 012341f779 outdated

43+        node = self.nodes[0]
44+
45+        self.log.info('Start with empty mempool, and 200 blocks')
46+        self.privkeys = [node.get_deterministic_priv_key().key]
47+        self.address = node.get_deterministic_priv_key().address
48+        self.coins = []

jnewbery commented at 4:07 pm on June 15, 2021:

No need for these to be member variables:

0        privkeys = [node.get_deterministic_priv_key().key]
1        address = node.get_deterministic_priv_key().address
2        coins = []

glozow commented at 8:02 am on June 16, 2021:

Done - got rid of some of it entirely since not needed for generate()

in test/functional/mempool_wtxid.py:50 in 012341f779 outdated

45+        self.log.info('Start with empty mempool, and 200 blocks')
46+        self.privkeys = [node.get_deterministic_priv_key().key]
47+        self.address = node.get_deterministic_priv_key().address
48+        self.coins = []
49+        # The last 100 coinbase transactions are premature
50+        for b in node.generatetoaddress(110, self.address)[:-100]:

jnewbery commented at 4:08 pm on June 15, 2021:

I think you can just call node.generate() and it’ll generate to the node’s deterministic address.

glozow commented at 7:38 am on June 16, 2021:

Ohhh I didn’t know that, I’ve been using generatetoaddress this whole time :scream:

glozow commented at 8:01 am on June 16, 2021:

Done

in test/functional/mempool_wtxid.py:106 in 012341f779 outdated

101+        self.log.info("Submit one child to the mempool")
102+        txid_submitted = node.sendrawtransaction(ToHex(child_one))
103+        assert_equal(node.getrawmempool(True)[txid_submitted]['wtxid'], child_one_wtxid)
104+
105+        # testmempoolaccept reports the "already in mempool" error
106+        assert_equal(node.testmempoolaccept([ToHex(child_one)]),

jnewbery commented at 4:25 pm on June 15, 2021:

can you break this into multiple lines like the assert_equal() below?

glozow commented at 8:01 am on June 16, 2021:

Done

jnewbery commented at 4:33 pm on June 15, 2021: member

Code review ACK 012341f779f4b82a757f30ef36810d8f8a1975dc

A few style comments inline. Feel free to ignore.

glozow commented at 4:52 pm on June 15, 2021: member

CC @ariard - The same-txid-different-wtxid issue was initially pointed out in #19645 and the test commit is converted from https://github.com/bitcoin/bitcoin/pull/19645/commits/d86e7a17081af04ceabe93b15a87cf295a9e9d84

DrahtBot commented at 8:25 pm on June 15, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22261 by jnewbery
#22252 by glozow

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.

in src/node/transaction.cpp:53 in 012341f779 outdated

49@@ -50,7 +50,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
50         // So if the output does exist, then this transaction exists in the chain.
51         if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN;
52     }
53-    if (!node.mempool->exists(hashTx)) {
54+    if (!node.mempool->exists(GenTxid(false, tx->GetHash()))) {

ariard commented at 0:08 am on June 16, 2021:

Is this change altering behavior of BroadcastTransaction ? Otherwise maybe better to just rename hashTx to txid, update comment to “Txid is not already in the mempool. Note if the submitted candidate differs by wtxid from an already present transaction, it’s for now rejected”, and make it its own commit as it’s only a documenting change ?

glozow commented at 8:01 am on June 16, 2021:

Good point, my intention is merely to highlight/document what the current behavior is. I’ve now marked the commits with “changes / doesn’t change behavior” and switched this to just be a comment explaining the behavior.

in test/functional/mempool_wtxid.py:8 in 012341f779 outdated

0@@ -0,0 +1,123 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2021 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""
6+Test mempool acceptance in case of an already known transaction
7+with identical non-witness data different witness.
8+"""

ariard commented at 0:13 am on June 16, 2021:

nit: I think the test could be called “mempool_wtxid_acceptance” because this is what it does exercising our mempool logic w.r.t to accepting candidates identified by wtxid imo ?

glozow commented at 8:00 am on June 16, 2021:

Done!

in src/validation.cpp:596 in 012341f779 outdated

617+        // Exact transaction already exists in the mempool.
618         return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool");
619+    } else if (m_pool.exists(GenTxid(false, tx.GetHash()))) {
620+        // Transaction with the same nonwitness data but different witnes (same txid, different
621+        // wtxid) already exists in the mempool. TODO: allow replacements
622+        return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-same-nonwitness-data-in-mempool");

ariard commented at 0:22 am on June 16, 2021:

What do you think about “mutant-replacement-disallowed” as a rejection message ? Where a mutant is defined as a transaction sharing an equivalent txid of an already in-mempool transaction but offering differing witnesses.

It’s quite kind of a glossary point but i don’t think we have yet an unified term across the codebase to designate same-txid-diff-wtxid ? Maybe someone has a better naming proposal, but it sounds less verbose rather than duplicating “same nonwitness data but different witness (same txid, different wtxid)” everywhere in the code paths where this subtlety is present.

glozow commented at 7:27 am on June 16, 2021:

I don’t think a user would find “mutant-replacement-disallowed” more helpful than “txn-same-nonwitness-data-in-mempool.” It would require them knowing what mutant replacement is (which doesn’t exist yet), which requires knowing what a mutant is (which is not widely defined). It’s confusing because it can imply that mutant replacement is allowed somehwere else…

If/when the replacement policy is implemented, that would be an excellent time to give it a name. But at this point, what’s relevant to users is “a different transaction with the same nonwitness data is already in the mempool.”

ariard commented at 6:46 pm on June 17, 2021:

Fair point, it’s quite a chicken-and-egg issue, either you pollute the codebases with multiple times verbose variable names like “same-txid-different-wtxid”, which are less unambiguous or you find one name, defined somehwhere in the codebase, and hope to reviewers to refer it during discussion.

Yes it can be added latter for sure, once we add mutant replacement.

in src/validation.h:185 in 012341f779 outdated

180@@ -181,12 +181,15 @@ struct MempoolAcceptResult {
181     const std::optional<std::list<CTransactionRef>> m_replaced_transactions;
182     /** Raw base fees in satoshis. */
183     const std::optional<CAmount> m_base_fees;
184+    /** Size in virtual bytes. */
185+    const std::optional<int64_t> m_vsize;

ariard commented at 0:29 am on June 16, 2021:

Do you plan to use vbytes as an identifier to dissociate same-txid-different-wtxid transaction in AcceptSingleTransaction consumers ?

I’m not sure it’s absent of collisions, two mutants can have the same vbytes but yet different witnesses ? E.g, a P2WSH 1-of-2 spent by either Alice signature or Bob signature

glozow commented at 7:23 am on June 16, 2021:

Nope. See https://github.com/bitcoin/bitcoin/pull/22252/commits/188cab0af2c5ee646f1467bd0a300d361ffd1fed - we need the vsize in the result because otherwise the RPC code doesn’t know it without looking into the mempool (which would require holding the mempool lock).

ariard commented at 6:43 pm on June 17, 2021:

I don’t think the commit you’re pointing to me is a unittest, exercising this logic with a collision on vbytes ? Do you think we can’t have vbytes collision among “same-txid-different-wtxids” candidates ?

we need the vsize in the result because otherwise the RPC code doesn’t know it without looking into the mempool (which would require holding the mempool lock).

Sorry, I don’t understand, what the RPC code is searching to know, the vbyte’s size or an ambiguous identifer between “same-txid-different-wtxids” candidates ?

ariard commented at 0:40 am on June 16, 2021: member

So I’m Concept ACK on returning a different testmempoolaccept error message when we hit a same-txid-different-wtxid mempool candidate but I don’t think this is necessary for current package mempool accept project ? Enabling witness replacement is a project which is worthy on its own imho, as Taproot make it more probable to have concurrent broadcasts with “feerate-divergent” witnesses.

Note, to allow same-txid-different-wtxid we still need to rework RBF rules, as the rule 5 enforces a replacement penalty which imply an absolute fee increases IIRC. However, an absolute fee is implicitly committed as part of non-witness data (sub outpoints’s amount/outputs’s value).

glozow force-pushed on Jun 16, 2021

in src/node/transaction.cpp:58 in 81d92fac91 outdated

50@@ -51,7 +51,11 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
51         if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN;
52     }
53     if (!node.mempool->exists(hashTx)) {
54-        // Transaction is not already in the mempool.
55+        // A transaction with the same txid is not already in the mempool. Note that it is still
56+        // possible for a transaction with same nonwitness data but different witness to be in the
57+        // mempool - in that case, we treat them as the same transaction and don't try to submit tx
58+        // to mempool. This logic must be changed if we allow replacements of
59+        // same-txid-different-wtxid transactions in the future.

jnewbery commented at 9:40 am on June 16, 2021:

I find this comment quite confusing. I initially read the “Note that it is still possible for a transaction with same nonwitness data but different witness to be in the mempool.” as meaning “it is possible for this condition to be true in this code branch”.

Does this seem better to you:

 0-    if (!node.mempool->exists(hashTx)) {
 1-        // A transaction with the same txid is not already in the mempool. Note that it is still
 2-        // possible for a transaction with same nonwitness data but different witness to be in the
 3-        // mempool - in that case, we treat them as the same transaction and don't try to submit tx
 4-        // to mempool. This logic must be changed if we allow replacements of
 5-        // same-txid-different-wtxid transactions in the future.
 6+    if (node.mempool->exists(hashTx)) {
 7+        // There's already a transaction in the mempool with this txid. Don't try to submit
 8+        // this transaction to the mempool, but do attempt to reannounce it if relay=true.
 9+        // The mempool transaction may have the same or different witness (and wtxid) as this
10+        // transaction.
11+    } else {

in test/functional/mempool_accept_wtxid.py:58 in 81d92fac91 outdated

53+                "amount": coinbase["vout"][0]["value"],
54+                "scriptPubKey": coinbase["vout"][0]["scriptPubKey"],
55+            })
56+        assert_equal(node.getmempoolinfo()['size'], 0)
57+
58+        txid = coins.pop()["txid"]

jnewbery commented at 9:42 am on June 16, 2021:

You don’t need a coins list or all this logic. Just generate 101 blocks, then get the txid of the first transaction in the tip block.

glozow force-pushed on Jun 16, 2021

jnewbery commented at 3:48 pm on June 16, 2021: member

ACK e6ba22edbc13f29d53f2dc0fcdcff49df879f597

ariard commented at 6:54 pm on June 17, 2021: member

@glozow

You know i’m quite bothered by you reaching offline, asking me to re-use my work with #19645 and the well-foundness of #22252, of which I said it wasn’t really required for a v0.1 package-relay and that we should be careful before considering this change.

I still appreciate the energy and dedication you’re pouring in this serie of work. That said next time if you have questions on any work related to better support for L2s, ping me on #bitcoin-core-dev. Anyone will be able to learn from the discussion, and it would avoid me to repeat twice my arguments :/

glozow commented at 10:21 am on June 18, 2021: member

@ariard We discussed this 8 months ago on github, when you posted that you would open the PR to make this change. I felt that there was a bit of urgency which is why I wanted to PR it, and messaged you privately to make sure we wouldn’t be doing redundant work. That’s my view, and I can also see that having multiple private/public communication channels can cause wires to cross and require repetitive messages. My apologies.

glozow closed this on Jun 18, 2021

glozow reopened this on Jun 18, 2021

glozow force-pushed on Jun 18, 2021

glozow commented at 10:59 am on June 18, 2021: member

I’ve updated the description and removed commits that aren’t strictly about same-txid-different-wtxid. Hopefully #22252 no longer needs to be considered for this PR! Also removed the commit for BroadcastTransaction() since #22261 covers it.

jnewbery commented at 12:26 pm on June 18, 2021: member

Code review ACK ea678026a7ca41659a09cab9ab5b989eed7d3bf9

darosior approved

darosior commented at 2:31 pm on June 18, 2021: member

utACK ea678026a7ca41659a09cab9ab5b989eed7d3bf9

in src/validation.cpp:618 in e64776154f outdated

615-    if (m_pool.exists(hash)) {
616+    if (m_pool.exists(GenTxid(true, tx.GetWitnessHash()))) {
617+        // Exact transaction already exists in the mempool.
618         return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool");
619+    } else if (m_pool.exists(GenTxid(false, tx.GetHash()))) {
620+        // Transaction with the same nonwitness data but different witnes (same txid, different

naumenkogs commented at 12:22 pm on July 7, 2021:

typo: “witnes”

theStack commented at 4:07 pm on July 7, 2021:

nit: s/nonwitness/non-witness/ Rationale:

0$ git grep nonwitness | wc -l
10
2$ git grep non-witness | wc -l
339

glozow commented at 4:33 pm on July 7, 2021:

makes sense, will use non-witness from now on!

glozow commented at 8:44 am on July 8, 2021:

Done

glozow commented at 8:44 am on July 8, 2021:

Fixed

naumenkogs commented at 12:31 pm on July 7, 2021: member

The last commit has Antoine emails twice:

0Co-authored-by: Antoine Riard <ariard@student.42.fr>
1Co-authored-by: Antoine Riard <antoine.riard@gmail.com>

Not that I care much, just pointing out it might be something odd.

At a high level, I’m not getting lost with the justification of this in terms of 19645.

If the plan is to have replacement of different-witness in the future, then why adding this error in the first place here? Do we need this in the meanwhile, until different-witness-replacement is merged?

glozow commented at 12:47 pm on July 7, 2021: member

If the plan is to have replacement of different-witness in the future, then why adding this error in the first place here? Do we need this in the meanwhile, until different-witness-replacement is merged?

We are not adding an error; we are simply distinguishing between the two cases, returning different error messages, and adding a test for same-txid-different-wtxid.
The new error message is an improvement upon the current “txn-already-in-mempool” which, in the case of same-txid-different-witness, is inaccurate and confusing for users.
This is not in conflict with witness replacement; I hope that this speeds it up. :) Merging this first means that PRs that deal with same-txid-different-wtxid (hopefully) don’t all conflict with each other.

theStack commented at 4:16 pm on July 7, 2021: member

Concept ACK 📝

I think this needs a rebase, as there is a silent merge conflict – the helper function ToHex doesn’t exist anymore in master, since #22257 (commit https://github.com/bitcoin/bitcoin/pull/22257/commits/a79396fe5f8f81c78cf84117a87074c6ff6c9d95) has been merged.

jnewbery commented at 5:40 pm on July 7, 2021: member

Since you’re going to rebase, a few v smol suggestions for the test:

 0--- a/test/functional/mempool_accept_wtxid.py
 1+++ b/test/functional/mempool_accept_wtxid.py
 2@@ -7,46 +7,42 @@ Test mempool acceptance in case of an already known transaction
 3 with identical non-witness data different witness.
 4 """
 5 
 6-from test_framework.script import (
 7-    CScript,
 8-    OP_0,
 9-    OP_TRUE,
10-    OP_IF,
11-    OP_HASH160,
12-    OP_EQUAL,
13-    OP_ELSE,
14-    OP_ENDIF,
15-    hash160,
16-)
17 from test_framework.messages import (
18+    COIN,
19+    COutPoint,
20     CTransaction,
21     CTxIn,
22     CTxInWitness,
23     CTxOut,
24-    COutPoint,
25     sha256,
26-    COIN,
27-    ToHex,
28 )
29-from test_framework.test_framework import BitcoinTestFramework
30-from test_framework.util import (
31-    assert_equal,
32+from test_framework.p2p import P2PTxInvStore
33+from test_framework.script import (
34+    CScript,
35+    OP_0,
36+    OP_ELSE,
37+    OP_ENDIF,
38+    OP_EQUAL,
39+    OP_HASH160,
40+    OP_IF,
41+    OP_TRUE,
42+    hash160,
43 )
44+from test_framework.test_framework import BitcoinTestFramework
45+from test_framework.util import assert_equal
46 
47 class MempoolWtxidTest(BitcoinTestFramework):
48     def set_test_params(self):
49         self.num_nodes = 1
50         self.setup_clean_chain = True
51-        self.extra_args = [["-incrementalrelayfee=0"]]
52 
53     def run_test(self):
54         node = self.nodes[0]
55 
56-        self.log.info('Start with empty mempool, and 200 blocks')
57-        privkeys = [node.get_deterministic_priv_key().key]
58+        self.log.info("Start with empty mempool")
59         # The last 100 coinbase transactions are premature
60-        earliest_blockhash = node.generate(101)[0]
61-        txid = node.getblock(blockhash=earliest_blockhash, verbosity=2)["tx"][0]["txid"]
62+        blockhash = node.generate(101)[0]
63+        txid = node.getblock(blockhash=blockhash, verbosity=2)["tx"][0]["txid"]
64         assert_equal(node.getmempoolinfo()['size'], 0)
65 
66         self.log.info("Submit parent with multiple script branches to mempool")
67@@ -60,11 +56,12 @@ class MempoolWtxidTest(BitcoinTestFramework):
68         parent.vout.append(CTxOut(int(9.99998 * COIN), script_pubkey))
69         parent.rehash()
70 
71+        privkeys = [node.get_deterministic_priv_key().key]
72         raw_parent = node.signrawtransactionwithkey(hexstring=parent.serialize().hex(), privkeys=privkeys)['hex']
73         parent_txid = node.sendrawtransaction(hexstring=raw_parent, maxfeerate=0)
74         node.generate(1)
75 
76-        # Create a new segwit transaction with witness solving first branch
77+        # Create a new transaction with witness solving first branch
78         child_witness_script = CScript([OP_TRUE])
79         child_witness_program = sha256(child_witness_script)
80         child_script_pubkey = CScript([OP_0, child_witness_program])
81@@ -77,7 +74,7 @@ class MempoolWtxidTest(BitcoinTestFramework):
82         child_one_wtxid = child_one.getwtxid()
83         child_one_txid = child_one.rehash()
84 
85-        # Create another identical segwit transaction with witness solving second branch
86+        # Create another identical transaction with witness solving second branch
87         child_two = CTransaction()
88         child_two.vin.append(CTxIn(COutPoint(int(parent_txid, 16), 0), b""))
89         child_two.vout.append(CTxOut(int(9.99996 * COIN), child_script_pubkey))

[validation] distinguish same txid different wtxid in mempool

Changes behavior.

fdb48163bf

[test] submit same txid different wtxid as mempool tx

Co-authored-by: Antoine Riard <ariard@student.42.fr>
Co-authored-by: Antoine Riard <antoine.riard@gmail.com>

b7a8cd9963

glozow force-pushed on Jul 8, 2021

glozow commented at 8:44 am on July 8, 2021: member

Rebased and applied suggestions from #22253 (comment)

naumenkogs commented at 9:16 am on July 8, 2021: member

ACK b7a8cd9963e810264d3b45d0ad15af863965c47a

jnewbery commented at 10:58 am on July 8, 2021: member

Code review ACK b7a8cd9963e810264d3b45d0ad15af863965c47a

in test/functional/mempool_accept_wtxid.py:7 in b7a8cd9963

0@@ -0,0 +1,116 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2021 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""
6+Test mempool acceptance in case of an already known transaction
7+with identical non-witness data different witness.

theStack commented at 1:04 pm on July 9, 2021:

nit:

0with identical non-witness data but different witness.

in test/functional/mempool_accept_wtxid.py:102 in b7a8cd9963

 97+            "wtxid": child_one_wtxid,
 98+            "allowed": False,
 99+            "reject-reason": "txn-already-in-mempool"
100+        }])
101+        testres_child_two = node.testmempoolaccept([child_two.serialize().hex()])[0]
102+        assert_equal(testres_child_two, {

theStack commented at 1:05 pm on July 9, 2021:

nit, could be simplified (like for child_one above):

0        assert_equal(node.testmempoolaccept([child_two.serialize().hex()]), [{

in test/functional/mempool_accept_wtxid.py:83 in b7a8cd9963

78+        # Create another identical transaction with witness solving second branch
79+        child_two = CTransaction()
80+        child_two.vin.append(CTxIn(COutPoint(int(parent_txid, 16), 0), b""))
81+        child_two.vout.append(CTxOut(int(9.99996 * COIN), child_script_pubkey))
82+        child_two.wit.vtxinwit.append(CTxInWitness())
83+        child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', witness_script]

theStack commented at 1:16 pm on July 9, 2021:

possible dedup alternative, since the txs only differ in one part:

0        child_two = copy.deepcopy(child_one)
1        child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', witness_script]

theStack approved

theStack commented at 1:37 pm on July 9, 2021: member

Code-review ACK b7a8cd9963e810264d3b45d0ad15af863965c47a

I think the simplest parent witness script to create same-nonwitness-data txs would be OP_DROP OP_TRUE, with a different arbitrary witness stack element in each child transaction. OTOH multiple script branches are far more realistic, I guess that’s the primary scenario :)

Left some non-blocking nits below only regarding the test, feel free to ignore.

darosior approved

darosior commented at 2:04 pm on July 9, 2021: member

re-utACK b7a8cd9963e810264d3b45d0ad15af863965c47a

laanwj merged this on Jul 9, 2021

laanwj closed this on Jul 9, 2021

glozow deleted the branch on Jul 9, 2021

in src/validation.cpp:593 in b7a8cd9963

586@@ -587,9 +587,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
587     if (!CheckFinalTx(m_active_chainstate.m_chain.Tip(), tx, STANDARD_LOCKTIME_VERIFY_FLAGS))
588         return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-final");
589 
590-    // is it already in the memory pool?
591-    if (m_pool.exists(hash)) {
592+    if (m_pool.exists(GenTxid(true, tx.GetWitnessHash()))) {
593+        // Exact transaction already exists in the mempool.
594         return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool");
595+    } else if (m_pool.exists(GenTxid(false, tx.GetHash()))) {

jonatack commented at 4:25 pm on July 9, 2021:

(One nicety if you pass this way again.)

0-    if (m_pool.exists(GenTxid(true, tx.GetWitnessHash()))) {
1+    if (m_pool.exists(GenTxid(/* is_wtxid */ true, tx.GetWitnessHash()))) {
2         // Exact transaction already exists in the mempool.
3         return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool");
4-    } else if (m_pool.exists(GenTxid(false, tx.GetHash()))) {
5+    } else if (m_pool.exists(GenTxid(/* is_wtxid */ false, tx.GetHash()))) {

sidhujag referenced this in commit 2516e156e6 on Jul 10, 2021

MarcoFalke referenced this in commit 87257d860e on Aug 3, 2021

sidhujag referenced this in commit 4fb6f0ee51 on Aug 4, 2021

unknown approved

fanquake locked this on Aug 12, 2021

validation: distinguish between same tx and same-nonwitness-data tx in mempool #22253

Conflicts