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

glozow · 2021-06-15T14:24:27Z

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.

jnewbery

Code review ACK 012341f

A few style comments inline. Feel free to ignore.

src/validation.cpp

test/functional/mempool_wtxid.py

glozow · 2021-06-15T16:52:52Z

CC @ariard - The same-txid-different-wtxid issue was initially pointed out in #19645 and the test commit is converted from d86e7a1

DrahtBot · 2021-06-15T20:25:49Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[p2p/mempool] Two small fixes to node broadcast logic #22261 by jnewbery
policy: Trim Packages when transaction with same txid exists in mempool #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.

ariard

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

src/node/transaction.cpp

test/functional/mempool_wtxid.py

ariard · 2021-06-16T00:22:54Z

src/validation.cpp

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


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.

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

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.

ariard · 2021-06-16T00:29:34Z

src/validation.h

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


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

Nope. See 188cab0 - 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).

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 ?

src/node/transaction.cpp

test/functional/mempool_accept_wtxid.py

jnewbery · 2021-06-16T15:48:41Z

ACK e6ba22e

ariard · 2021-06-17T18:54:08Z

@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 · 2021-06-18T10:21:01Z

@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 · 2021-06-18T10:59:07Z

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 · 2021-06-18T12:26:33Z

Code review ACK ea67802

darosior

utACK ea67802

src/validation.cpp

naumenkogs · 2021-07-07T12:31:07Z

The last commit has Antoine emails twice:

Co-authored-by: Antoine Riard <ariard@student.42.fr>
Co-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 · 2021-07-07T12:47:58Z

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

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 a79396f) has been merged.

src/validation.cpp

jnewbery · 2021-07-07T17:40:15Z

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

Diff

--- a/test/functional/mempool_accept_wtxid.py
+++ b/test/functional/mempool_accept_wtxid.py
@@ -7,46 +7,42 @@ Test mempool acceptance in case of an already known transaction
 with identical non-witness data different witness.
 """
 
-from test_framework.script import (
-    CScript,
-    OP_0,
-    OP_TRUE,
-    OP_IF,
-    OP_HASH160,
-    OP_EQUAL,
-    OP_ELSE,
-    OP_ENDIF,
-    hash160,
-)
 from test_framework.messages import (
+    COIN,
+    COutPoint,
     CTransaction,
     CTxIn,
     CTxInWitness,
     CTxOut,
-    COutPoint,
     sha256,
-    COIN,
-    ToHex,
 )
-from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import (
-    assert_equal,
+from test_framework.p2p import P2PTxInvStore
+from test_framework.script import (
+    CScript,
+    OP_0,
+    OP_ELSE,
+    OP_ENDIF,
+    OP_EQUAL,
+    OP_HASH160,
+    OP_IF,
+    OP_TRUE,
+    hash160,
 )
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
 
 class MempoolWtxidTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         self.setup_clean_chain = True
-        self.extra_args = [["-incrementalrelayfee=0"]]
 
     def run_test(self):
         node = self.nodes[0]
 
-        self.log.info('Start with empty mempool, and 200 blocks')
-        privkeys = [node.get_deterministic_priv_key().key]
+        self.log.info("Start with empty mempool")
         # The last 100 coinbase transactions are premature
-        earliest_blockhash = node.generate(101)[0]
-        txid = node.getblock(blockhash=earliest_blockhash, verbosity=2)["tx"][0]["txid"]
+        blockhash = node.generate(101)[0]
+        txid = node.getblock(blockhash=blockhash, verbosity=2)["tx"][0]["txid"]
         assert_equal(node.getmempoolinfo()['size'], 0)
 
         self.log.info("Submit parent with multiple script branches to mempool")
@@ -60,11 +56,12 @@ class MempoolWtxidTest(BitcoinTestFramework):
         parent.vout.append(CTxOut(int(9.99998 * COIN), script_pubkey))
         parent.rehash()
 
+        privkeys = [node.get_deterministic_priv_key().key]
         raw_parent = node.signrawtransactionwithkey(hexstring=parent.serialize().hex(), privkeys=privkeys)['hex']
         parent_txid = node.sendrawtransaction(hexstring=raw_parent, maxfeerate=0)
         node.generate(1)
 
-        # Create a new segwit transaction with witness solving first branch
+        # Create a new transaction with witness solving first branch
         child_witness_script = CScript([OP_TRUE])
         child_witness_program = sha256(child_witness_script)
         child_script_pubkey = CScript([OP_0, child_witness_program])
@@ -77,7 +74,7 @@ class MempoolWtxidTest(BitcoinTestFramework):
         child_one_wtxid = child_one.getwtxid()
         child_one_txid = child_one.rehash()
 
-        # Create another identical segwit transaction with witness solving second branch
+        # Create another identical transaction with witness solving second branch
         child_two = CTransaction()
         child_two.vin.append(CTxIn(COutPoint(int(parent_txid, 16), 0), b""))
         child_two.vout.append(CTxOut(int(9.99996 * COIN), child_script_pubkey))

Changes behavior.

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

glozow · 2021-07-08T08:44:08Z

Rebased and applied suggestions from #22253 (comment)

naumenkogs · 2021-07-08T09:16:11Z

ACK b7a8cd9

jnewbery · 2021-07-08T10:58:40Z

Code review ACK b7a8cd9

theStack

Code-review ACK b7a8cd9

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.

theStack · 2021-07-09T13:04:25Z

test/functional/mempool_accept_wtxid.py

+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Test mempool acceptance in case of an already known transaction
+with identical non-witness data different witness.


nit:

Suggested change

with identical non-witness data different witness.

with identical non-witness data but different witness.

theStack · 2021-07-09T13:05:33Z

test/functional/mempool_accept_wtxid.py

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


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

Suggested change

testres_child_two = node.testmempoolaccept([child_two.serialize().hex()])[0]

assert_equal(testres_child_two, {

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

theStack · 2021-07-09T13:16:07Z

test/functional/mempool_accept_wtxid.py

+        child_two = CTransaction()
+        child_two.vin.append(CTxIn(COutPoint(int(parent_txid, 16), 0), b""))
+        child_two.vout.append(CTxOut(int(9.99996 * COIN), child_script_pubkey))
+        child_two.wit.vtxinwit.append(CTxInWitness())
+        child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', witness_script]


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

Suggested change

child_two = CTransaction()

child_two.vin.append(CTxIn(COutPoint(int(parent_txid, 16), 0), b""))

child_two.vout.append(CTxOut(int(9.99996 * COIN), child_script_pubkey))

child_two.wit.vtxinwit.append(CTxInWitness())

child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', witness_script]

child_two = copy.deepcopy(child_one)

child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', witness_script]

darosior

re-utACK b7a8cd9

jonatack · 2021-07-09T16:25:14Z

src/validation.cpp

        return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool");
+    } else if (m_pool.exists(GenTxid(false, tx.GetHash()))) {


(One nicety if you pass this way again.)

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

…-nonwit…

91b0597 Improve mempool_accept_wtxid.py (naiza) Pull request description: Follow-up to #22253 adding changes suggested in [#22253 (review)](#22253 (comment)) ACKs for top commit: glozow: utACK 91b0597 Tree-SHA512: 383064138a5b2160d769c9df370470fd585c91682083013a6fa15e14448a4b481bc09b3a0ed6e75554db2c378df6b2263c65f209f973c9e9d577e15814a4be1d

91b0597 Improve mempool_accept_wtxid.py (naiza) Pull request description: Follow-up to bitcoin#22253 adding changes suggested in [bitcoin#22253 (review)](bitcoin#22253 (comment)) ACKs for top commit: glozow: utACK 91b0597 Tree-SHA512: 383064138a5b2160d769c9df370470fd585c91682083013a6fa15e14448a4b481bc09b3a0ed6e75554db2c378df6b2263c65f209f973c9e9d577e15814a4be1d

glozow mentioned this pull request Jun 15, 2021

policy: Trim Packages when transaction with same txid exists in mempool #22252

Closed

glozow changed the title ~~[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 Jun 15, 2021

glozow added RPC/REST/ZMQ Validation labels Jun 15, 2021

jnewbery reviewed Jun 15, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

test/functional/mempool_wtxid.py Outdated Show resolved Hide resolved

test/functional/mempool_wtxid.py Outdated Show resolved Hide resolved

test/functional/mempool_wtxid.py Outdated Show resolved Hide resolved

ariard reviewed Jun 16, 2021

View reviewed changes

glozow force-pushed the 2021-06-same-txid-diff-wtxid branch from 012341f to 81d92fa Compare June 16, 2021 07:55

jnewbery reviewed Jun 16, 2021

View reviewed changes

src/node/transaction.cpp Outdated Show resolved Hide resolved

test/functional/mempool_accept_wtxid.py Outdated Show resolved Hide resolved

glozow force-pushed the 2021-06-same-txid-diff-wtxid branch from 81d92fa to e6ba22e Compare June 16, 2021 12:36

DrahtBot mentioned this pull request Jun 16, 2021

[p2p/mempool] Two small fixes to node broadcast logic #22261

Merged

glozow closed this Jun 18, 2021

glozow reopened this Jun 18, 2021

glozow force-pushed the 2021-06-same-txid-diff-wtxid branch from e6ba22e to ea67802 Compare June 18, 2021 10:54

darosior approved these changes Jun 18, 2021

View reviewed changes

glozow mentioned this pull request Jun 20, 2021

Package Mempool Submission with Package Fee-Bumping #22290

Closed

10 tasks

naumenkogs reviewed Jul 7, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

theStack reviewed Jul 7, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

glozow and others added 2 commits July 8, 2021 09:31

[validation] distinguish same txid different wtxid in mempool

fdb4816

Changes behavior.

[test] submit same txid different wtxid as mempool tx

b7a8cd9

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

glozow force-pushed the 2021-06-same-txid-diff-wtxid branch from ea67802 to b7a8cd9 Compare July 8, 2021 08:41

theStack approved these changes Jul 9, 2021

View reviewed changes

darosior approved these changes Jul 9, 2021

View reviewed changes

laanwj merged commit 8ab0c77 into bitcoin:master Jul 9, 2021

glozow deleted the 2021-06-same-txid-diff-wtxid branch July 9, 2021 15:47

jonatack reviewed Jul 9, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 10, 2021

Merge bitcoin#22253: validation: distinguish between same tx and same…

2516e15

…-nonwit…

naiza2000 mentioned this pull request Jul 22, 2021

test : improve mempool_accept_wtxid.py #22532

Merged

ghost approved these changes Aug 12, 2021

View reviewed changes

bitcoin locked and limited conversation to collaborators Aug 12, 2021

	with identical non-witness data different witness.
	with identical non-witness data but different witness.

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

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

		return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool");
		} else if (m_pool.exists(GenTxid(false, tx.GetHash()))) {

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

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

Uh oh!

Conversation

glozow commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow commented Jun 15, 2021

Uh oh!

DrahtBot commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jun 16, 2021

Uh oh!

ariard commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jun 18, 2021

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naumenkogs commented Jul 7, 2021

Uh oh!

glozow commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Jul 7, 2021

Uh oh!

glozow commented Jul 8, 2021

Uh oh!

naumenkogs commented Jul 8, 2021

Uh oh!

jnewbery commented Jul 8, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow commented Jun 15, 2021 •

edited

Loading

DrahtBot commented Jun 15, 2021 •

edited

Loading

ariard commented Jun 17, 2021 •

edited

Loading

glozow commented Jun 18, 2021 •

edited

Loading

glozow commented Jun 18, 2021 •

edited

Loading

glozow commented Jul 7, 2021 •

edited

Loading