We want wtxid relay to be widely deployed before taproot activation, so it should be backported to v0.20.
The main difference from #18044 is removing the changes to the unbroadcast set (which was only added post-v0.20). The rest is mostly minor rebase conflicts (eg connman changed from a pointer to a reference in master, etc).
We’ll also want to backport #19569 after that’s merged.
344@@ -343,6 +345,7 @@ def on_reject(self, message): pass
345 def on_sendcmpct(self, message): pass
346 def on_sendheaders(self, message): pass
347 def on_tx(self, message): pass
348+ def on_wtxidrelay(self, message): pass
349
350 def on_inv(self, message):
351 want = msg_getdata()
Note: two below here does not get changed to checking for
0+ if (i.type == MSG_TX) or (i.type == MSG_WTX):
as in master PR. Tests seem to still work when I make the change.
code review ACK 91646ffc2ad07bf42bf71293940026ddebe31960
All differences either related to the unbroadcast set work not existing, some inner help functions for mempool data retrieval, or adapting tests.
Did some basic spot-checking of the functional testing even though it matched the master PR, and fails when expected.
Looks like wallet_resendwallettransactions.py is having the sads sometimes in travis:
0 File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_resendwallettransactions.py", line 47, in run_test
1 wtxid = int(node.getrawtransaction(hex(txid)[2:], 1)['hash'], 16)
2 File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 47, in __call__
3 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
4 File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 141, in __call__
5 raise JSONRPCException(response['error'], status)
6test_framework.authproxy.JSONRPCException: parameter 1 must be of length 64 (not 62, for '5c0bf5afe173cc9e2fc9dcf07708219f57d0543759544b390005f6e581075a') (-8)
43@@ -40,6 +44,7 @@ def run_test(self):
44
45 self.log.info("Create a new transaction and wait until it's broadcast")
46 txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16)
47+ wtxid = int(node.getrawtransaction(hex(txid)[2:], 1)['hash'], 16)
This is dropping the padding left zeroes I think with hex(txid)[2:] converting a smaller int to hex.
Just save txid above as a string, then convert to int after this line and use it directly, or:
0"{0:0{1}x}".format(txid, 64)
(I think former is easier :P )
0x. Will fix.
0x05 or something rather than 0x00000000....05
int
1315@@ -1295,8 +1316,6 @@ def test_tx_relay_after_segwit_activation(self):
1316 # Node will not be blinded to the transaction
1317 self.std_node.announce_tx_and_wait_for_getdata(tx3)
1318 test_transaction_acceptance(self.nodes[1], self.std_node, tx3, True, False, 'tx-size')
In 59f38ad8be2fdbbe2c6cc23bd8e6f226ac493d76
Hm, here I deleted that test by accident which @instagibbs added back, see #19649. Might want to not remove it here as well.
Code review ACK 4daad798f3b5ea258cb183932956101e8df24e94 modulo that removed test, although I am not sure if this is a blocker for a backport.
Checked the diffs with git range-diff 2b4b90aa8f0440deacefb5997d7bd1f9f5c591b3~1..0a4f1422cd1c20e12a05d7ff1a2ef1d5e7c654bb 0615a589dd70afd7280921b7e32f96c1e018cb99~1..4daad798f3b5ea258cb183932956101e8df24e94 and looked at the commits with relevant changes in more detail.
Rebased
Specifically, there were conflicts around the conditionals for adding wtxid/txids to recent rejects in commits Add wtxids to recentRejects and Make TX_WITNESS_STRIPPED its own rejection reason.
If it’s too difficult to review those changes, we could revert #19680 so these are merged into V0.20 in the same order as master.
Code review ACK f5d90eadc182a2a3d1cffa4cc01b4fb68f9dc567
Confirmed relevant commits to review: git range-diff 0615a589dd70afd7280921b7e32f96c1e018cb99~1..4daad798f3b5ea258cb183932956101e8df24e94 220a18c3ca31ac542b08a6db402c79d20625feca~1..f5d90eadc182a2a3d1cffa4cc01b4fb68f9dc567 then checked these individually and also compared result to current master.
ACK f5d90eadc182a2a3d1cffa4cc01b4fb68f9dc567 on non-python commits. Verified by cherry-picking commits one-by-one from #18044 on top of the 0.20 branch.
This piece of code https://github.com/bitcoin/bitcoin/blob/f5d90eadc182a2a3d1cffa4cc01b4fb68f9dc567/src/net_processing.cpp#L1971-L1975 has an explaining comment, but this one https://github.com/bitcoin/bitcoin/blob/f5d90eadc182a2a3d1cffa4cc01b4fb68f9dc567/src/net_processing.cpp#L2755-L2757 lacks it. Is it intentionally?
Since it's only used for transactions, there's no need to pass in an inv type.
This is in preparation for wtxid-based invs (we need to be able to tell whether
we AlreadyHave() a transaction based on either txid or wtxid).
This also double the size of the bloom filter, which is overkill, but still
uses a manageable amount of memory.
Previously, we only added txids to recentRejects if we were sure that the
transaction couldn't have had the wrong witness (either because the witness was
malleated or stripped).
In preparation for wtxid-based relay, we can observe that txid == wtxid for
transactions that have no witness, and add the wtxid of rejected transactions,
provided the transaction wasn't a witness-stripped one. This means that we now
add more data to the filter (as prior to this commit, any transaction with a
witness that failed to be accepted was being skipped for inclusion in the
filter) but witness malleation should still not interfere with relay of a valid
segwit transaction, because the txid of a segwit transaction would not be added
to the filter after failing validation.
In the future, having wtxids in the recent rejects filter will allow us to
skip downloading the same wtxid multiple times, once our peers use wtxids for
transaction relay.
Also add the txid to recentRejects if the transaction failed for
TX_INPUTS_NOT_STANDARD.
This adds a field to CNodeState that tracks whether to relay transactions with
that peer via wtxid, instead of txid. As of this commit the field will always
be false, but in a later commit we will add a way to negotiate turning this on
via p2p messages exchanged with the peer.
When sent to and received from a given peer, enables using wtxid's for
announcing and fetching transactions with that peer.
Using both txid and wtxid-based relay with peers means that we could sometimes
download the same transaction twice, if announced via two different hashes from
different peers.
Use a heuristic of delaying txid-peer-getdata requests by 2 seconds, if we have
at least one wtxid-based peer.
Previously, TX_WITNESS_MUTATED could be returned during transaction validation
for either transactions that had a witness that was non-standard, or for
transactions that had no witness but were invalid due to segwit validation
rules.
However, for txid/wtxid-relay considerations, net_processing distinguishes the
witness stripped case separately, because it affects whether a wtxid should be
able to be added to the reject filter. It is safe to add the wtxid of a
witness-mutated transaction to the filter (as that wtxid shouldn't collide with
the txid, and hence it wouldn't interfere with transaction relay from
txid-relay peers), but it is not safe to add the wtxid (== txid) of a
witness-stripped transaction to the filter, because that would interfere with
relay of another transaction with the same txid (but different wtxid) when
relaying from txid-relay peers.
Also updates the comment explaining this logic, and explaining that we can get
rid of this complexity once there's a sufficient deployment of wtxid-relaying
peers on the network.
This new p2p protocol version allows to use WTXIDs for tx relay.
Also cleans up some doublicate lines in the rest of the test.
co-authored-by: Anthony Towns <aj@erisian.com.au>
re-ACK d4a1ee8f1d4c46ab726be83965bd86bace2ec1ec
Confirmed no changes since my last review: git range-diff 220a18c3ca31ac542b08a6db402c79d20625feca~1..f5d90eadc182a2a3d1cffa4cc01b4fb68f9dc567 80aa83aa406447d9b0932301b37966a30d0e1b6e..d4a1ee8f1d4c46ab726be83965bd86bace2ec1ec
re-ACK d4a1ee8f1d4c46ab726be83965bd86bace2ec1ec, only rebased since my previous review:
0$ git range-diff master f5d90eadc182a2a3d1cffa4cc01b4fb68f9dc567 d4a1ee8f1d4c46ab726be83965bd86bace2ec1ec
1 1: 004b0e1b9 = 1: 004b0e1b9 build: Bump version to 0.20.0
2 2: cd0c99890 = 2: cd0c99890 doc: Update manpages pre-rc1
3 3: cd1f5bf1d = 3: cd1f5bf1d qt: Update translations pre-rc1
4 4: ade4185e6 = 4: ade4185e6 gitian: Add missing automake package to gitian-win-signer.yml
5 5: 842b13a5f = 5: 842b13a5f Avoid non-trivial global constants in SHA-NI code
6 6: 1d1e3585f = 6: 1d1e3585f build: Set libevent minimum version to 2.0.21
7 7: 6986b2634 = 7: 6986b2634 build: fix ASLR for bitcoin-cli on Windows
8 8: 54d2063d1 = 8: 54d2063d1 Do not expose and consider -logthreadnames when it does not work
9 9: a9ca65bd2 = 9: a9ca65bd2 Fix naming of macOS SDK and clarify version
1010: 7f7548d82 = 10: 7f7548d82 rpc: Do not advertise dumptxoutset as a way to flush the chainstate
1111: 59d57f6c1 = 11: 59d57f6c1 build: Ensure source tarball has leading directory name
1212: 7d87ba0e0 = 12: 7d87ba0e0 travis: Remove valgrind
1313: aa7c6858e = 13: aa7c6858e travis: Remove s390x
1414: 315ae14f3 = 14: 315ae14f3 gui: Fix itemWalletAddress leak when not tree mode
1515: fb821731e = 15: fb821731e [net processing] ignore tx GETDATA from blocks-only peers
1616: 1e73d7248 = 16: 1e73d7248 [net processing] ignore unknown INV types in GETDATA messages
1717: 011532e38 = 17: 011532e38 [test] test that an invalid GETDATA doesn't prevent processing of future messages
1818: a3fe458a1 = 18: a3fe458a1 [docs] Improve commenting in ProcessGetData()
1919: ca4dac48c = 19: ca4dac48c rpc: Add mutex to guard deadlineTimers
2020: 251e321ad = 20: 251e321ad rpc: Relock wallet only if most recent callback
2121: ed0afe8c1 = 21: ed0afe8c1 test: Add test for conflicted wallet tx notifications
2222: ff4dc2075 = 22: ff4dc2075 gui: Fix manual coin control with multiple wallets loaded
2323: 37a620748 = 23: 37a620748 test: Add unregister_validation_interface_race test
2424: cc7d34465 = 24: cc7d34465 miner: Avoid stack-use-after-return in validationinterface
2525: cf2a6e2a3 = 25: cf2a6e2a3 test: Remove const to work around compiler error on xenial
2626: 6161c94a6 = 26: 6161c94a6 [net processing] Only send a getheaders for one block in an INV
2727: 9a8fb4cf4 = 27: 9a8fb4cf4 fuzz: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer
2828: 245c862cf = 28: 245c862cf test: disable script fuzz tests
2929: 0793eca3a = 29: 0793eca3a qt: Pre-rc2 translations update
3030: 6f7f94a27 = 30: 6f7f94a27 build: Bump RC to rc2
3131: 1dfad4259 = 31: 1dfad4259 doc: Merge 0.20.0 release notes from wiki
3232: 808c8d15f = 32: 808c8d15f build: Set rc to 0 for -final
3333: a62f0ed64 = 33: a62f0ed64 doc: Manpages update pre-final
3434: 384d3f991 = 34: 384d3f991 Add missing QPainterPath include
3535: 0596a6eeb = 35: 0596a6eeb util: Don't reference errno when pthread fails.
3636: c219d2163 = 36: c219d2163 build: improved output of configure for build OS
3737: 5c7151a60 = 37: 5c7151a60 gui: update Qt base translations for macOS release
3838: febebc4ea = 38: febebc4ea Fix WSL file locking by using flock instead of fcntl
3939: 654420d6d = 39: 654420d6d wallet: Minimal fix to restore conflicted transaction notifications
4040: 27786d072 = 40: 27786d072 trivial: Suggested cleanups to surrounding code
4141: 68e0e6f85 = 41: 68e0e6f85 rpc: show both UTXOs in decodepsbt
4242: ed5ec3080 = 42: ed5ec3080 psbt: Allow both non_witness_utxo and witness_utxo
4343: 3228b59b1 = 43: 3228b59b1 psbt: always put a non_witness_utxo and don't remove it
4444: cf0b5a933 = 44: cf0b5a933 tests: Check that segwit inputs in psbt have both UTXO types
4545: c9b49d285 = 45: c9b49d285 wallet: Handle concurrent wallet loading
4646: eb6b82a55 = 46: eb6b82a55 qa: Test concurrent wallet loading
4747: e7f06f9b0 = 47: e7f06f9b0 test: remove Cirrus CI FreeBSD job
4848: 047734805 = 48: 047734805 Replace automatic bans with discouragement filter
4949: 2b79ac740 = 49: 2b79ac740 Clean up separated ban/discourage interface
5050: fa3998859 = 50: fa3998859 doc: Clear release notes for minor release
5151: 888886ed5 = 51: 888886ed5 doc: move-only release notes for 0.20.1
5252: faf5e256c = 52: faf5e256c appveyor: Remove clcache
5353: fae0e9383 = 53: fae0e9383 Remove cached directories and associated script blocks from appveyor CI configuration.
5454: bad9cf8f4 = 54: bad9cf8f4 Increment input value sum only once per UTXO in decodepsbt
5555: 8b4093749 = 55: 8b4093749 qt: Fix QFileDialog for static builds
5656: cd34ff546 = 56: cd34ff546 build: Bump version to 0.20.1rc1
5757: 5e21c55ef = 57: 5e21c55ef doc: Regenerate man pages for 0.20.1rc1
5858: cac7a9809 = 58: cac7a9809 qt: Translation update for 0.20.1rc1
5959: 58feb9ecb = 59: 58feb9ecb doc: Add changelog and authors to release notes for 0.20.1
6060: 7ee4769cd = 60: 7ee4769cd [0.20] lint: fix shellcheck URL in CI install
6161: 7ff64311b = 61: 7ff64311b build: Bump version to 0.20.1-final
6262: 7c1c15329 = 62: 7c1c15329 doc: Update 0.20.1 release notes with psbt changes
6363: 06f9c5c3b = 63: 06f9c5c3b Add txids with non-standard inputs to reject filter
6464: 107cf1515 = 64: 107cf1515 test addition of unknown segwit spends to txid reject filter
65 -: --------- > 65: ad99777b5 Set appveyor vm version to previous Visual Studio 2019 release.
66 -: --------- > 66: 498b7cb6f Update the vcpkg checkout commit ID in appveyor config.
6765: 220a18c3c = 67: 4df3d139b Add a wtxid-index to the mempool
6866: 7cedceb60 = 68: f7833b5bd Just pass a hash to AddInventoryKnown
6967: b6a583851 = 69: 365493767 Add a wtxid-index to mapRelay
7068: 735be4534 = 70: 606755b84 Add wtxid-index to orphan map
7169: c695be95a = 71: 73845211d Add wtxids of confirmed transactions to bloom filter
7270: 36c340238 = 72: be1b7a891 Add wtxids to recentRejects
7371: 91b2e8074 = 73: 2599277e9 Add support for tx-relay via wtxid
7472: ab2ce25b5 = 74: 93826726e ignore non-wtxidrelay compliant invs
7573: 3ae27adca = 75: 181ffadd1 Add p2p message "wtxidrelay"
7674: fa93551fe = 76: c1d6a1003 Delay getdata requests from peers using txid-based relay
7775: 76de3f897 = 77: 879a3cf2c Make TX_WITNESS_STRIPPED its own rejection reason
7876: f53543078 = 78: e364b2a2d Rename AddInventoryKnown() to AddKnownTx()
7977: 5e33fe096 = 79: 6be398b6f test: Update test framework p2p protocol version to 70016
8078: 9551605e5 = 80: e48168196 test: Add tests for wtxid tx relay in segwit test
8179: ebd00f4af = 81: 22effa51a test: Use wtxid relay generally in functional tests
8280: cbb581bdb = 82: f082a13ab Disconnect peers sending wtxidrelay message after VERACK
8381: f5d90eadc = 83: d4a1ee8f1 Further improve comments around recentRejects