Enable fetching of orphan parents from wtxid peers #19569

sipa · 2020-07-23T00:59:22Z

A new type GenTxid is added to protocol.h, which represents a tagged txid-or-wtxid. The tx request logic is updated to use these instead of uint256s, permitting per-announcement distinguishing of txid/wtxid (instead of assuming that everything we want to request from a wtxid peer is wtx). Then the restriction of orphan-parent requesting to non-wtxid peers is lifted.

Also document BIP339 in doc/bips.md.

sipa · 2020-07-23T01:04:05Z

@ajtowns I made a few changes compared to your branch:

The pair type is a bit richer, and in primitives/transaction.h (so that it's available from txrequest in Overhaul transaction request logic #19184).
All of the txrequest data structures use it, not just m_tx_process_time - this feels more obviously correct.
The restriction that wtxidrelay peers can only announce using MSG_WTX invs remains - this isn't violated by txid-based orphan parent requests (which are get GETDATAs, not INVs).

Let me know what you think.

src/net_processing.cpp

ajtowns

All of the txrequest data structures use it, not just m_tx_process_time - this feels more obviously correct.

👍

The restriction that wtxidrelay peers can only announce using MSG_WTX invs remains - this isn't violated by txid-based orphan parent requests (which are get GETDATAs, not INVs).

Ha. I thought when I was writing that patch that removing that restriction wasn't supposed to be necessary.

Code review ACK 154778d

src/primitives/transaction.h

src/net_processing.cpp

naumenkogs · 2020-07-23T09:19:30Z

utACK 154778d

jonatack

ACK 154778d code review, debug build/tests/ran node, checked test here fails on master and reverted test fails on the branch.

Reviewers: suggest reviewing 54a0936 with -w.

doc/bips.md

jonatack · 2020-07-24T08:08:11Z

ACK 338f80e per git range-diff 007e15dc 154778d 338f80e and re-code review. Re-debug build with Clang 12, ran tests and node.

src/net_processing.cpp

ariard · 2020-07-24T13:37:50Z

src/net_processing.cpp

-                    }
+                for (const CTxIn& txin : tx.vin) {
+                    // Here, we only have the txid (and not wtxid) of the
+                    // inputs, so we only request in txid mode, even for


Strictly interpreting the BIP, the requirement for wtxid-relay on fetching is scoped to "announced transactions from that peer", and orphan parents have not been announced so we don't violate wtxid-relay policy here. Concern is on future BIP implementers on how they interpret "is required" and severing the connection for violations. Lightweight clients broadcasting a parent + CPFP tx may know issues due to this.

In the meanwhile, I share Suhas point to not commit a given handling of this behavior. Maybe we should amend the BIP to invite liberal enforcement of violations, just dropping non-complying invs/getdatas ?

sdaftuar

Just did a code review and spotted one potential issue, but otherwise looks good. Will test.

src/net_processing.cpp

sipa · 2020-07-25T21:31:46Z

Changed the code to treat tx announcements as keyed by hash (rather than hash+wtxidness), so that txid requests for parents of unconfirmed transactions won't result in a duplicate query if a wtxid with the same hash is already in flight. This reduces the diff a bit as well, and removed the need for ToString().

jnewbery · 2020-07-26T08:28:01Z

I have a hard time reasoning about the consistency of the various data structures around tx requesting if we don't.

I 100% agree with this. Net processing is complicated enough as it is without having data structures hold a mixture of different types. txids and wtxids are different types that just happen to both be encoded in a uint256, which means the compiler can't protect us from bugs where we mistake one for the other.

If we're going to do this, we should at least make it very clear to the programmer by adding comments to the data structures and functions that are doing this. For example, EraseTxRequest(), GetTxRequestTime()and UpdateTxRequestTime() all have a parameter named txid. Anyone reading/reviewing that code would therefore expect those parameters to be txids (and by extension, g_already_asked_for to be keyed by txid).

jonatack

ACK 707d0f4

I added a few CInv tx msg type boolean helpers on this branch to simplify ToGenTxid() and the net processing code, if you're inclined to use any of it: https://github.com/jonatack/bitcoin/commits/add-inv-msg-booleans (EDIT: opened #19590 for the parts non-specific to this branch).

src/net_processing.cpp

sipa · 2020-07-26T17:02:20Z

@jnewbery Would it help if the various txrequest-related functions take a GenTxid argument, but where appropriate, call GetHash() on it when they really just need a txid-or-wtxid key, instead of passing those keys around as uint256s?

sdaftuar · 2020-07-26T19:29:52Z

Code review ACK, apart from the test change which I haven't reviewed. Will test.

sdaftuar · 2020-07-26T21:43:43Z

ACK 707d0f4

src/net_processing.cpp

naumenkogs · 2020-07-27T09:19:42Z

utACK 707d0f4

DrahtBot · 2020-07-27T10:48:13Z

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: change CInv::type from int to uint32_t, fix UBSan warning #19611 (p2p: refactor CInv::type from public int to private uint32_t by jonatack)
p2p: refactor AlreadyHave(), CInv::type, INV/TX processing #19610 (p2p, refactor: add CInv block message helpers; use in net processing by jonatack)
Deduplicate parent txid loop of requested transactions and missing parents of orphan transactions #19596 (Deduplicate parent txid loop of requested transactions and missing parents of orphan transactions by sdaftuar)
Remove mempool global #19556 (Remove mempool global by MarcoFalke)
Implement ADDRv2 support (part of BIP155) #19031 (Implement ADDRv2 support (part of BIP155) by vasild)

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.

sipa · 2020-07-27T21:28:29Z

I added 3 extra commits that introduce more GenTxid uses, without changing behavior.

sdaftuar · 2020-07-27T22:50:42Z

utACK c048357

jnewbery

Code review ACK c048357. A few comments inline.

Thanks for updating the functions to take GenTxids. I think that's clearer. I still think that the original version of this PR was better still, and that storing GenTxids in the various tx download data structures is easier to reason about and worth the marginal loss in efficiency, but I guess that all this is going away with #19184 so it doesn't matter too much.

src/net_processing.cpp

src/protocol.h

src/net_processing.cpp

jonatack · 2020-07-30T06:48:34Z

Light ACK c048357 modulo Gleb's and John's feedback above.

src/net_processing.cpp

… logic

Based on a commit by Anthony Towns.

sipa · 2020-07-30T20:49:10Z

Rebased and addressed comments.

ajtowns · 2020-07-31T06:23:00Z

ACK 10b7a6d -- code review. Using gtxid to replace the is_txid_or_wtxid flag for the mempool functions is nice.

naumenkogs · 2020-07-31T07:19:52Z

utACK 10b7a6d

jonatack · 2020-07-31T08:06:28Z

ACK 10b7a6d

jnewbery · 2020-07-31T11:13:03Z

Code review ACK 10b7a6d

jnewbery · 2020-07-31T13:20:45Z

I have a backport of (the first 4 commits of) this to v0.20 here: https://github.com/jnewbery/bitcoin/tree/2020-07-v20-wtxid-orphan. It can be added to #19606 or PR'ed separately. Whatever is easiest for reviewers.

10b7a6d refactor: make txmempool interface use GenTxid (Pieter Wuille) 5c124e1 refactor: make FindTxForGetData use GenTxid (Pieter Wuille) a2bfac8 refactor: use GenTxid in tx request functions (Pieter Wuille) e65d115 test: request parents of orphan from wtxid relay peer (Anthony Towns) 900d7f6 p2p: enable fetching of orphans from wtxid peers (Pieter Wuille) 9efd86a refactor: add GenTxid (=txid or wtxid) type and use it for tx request logic (Pieter Wuille) d362f19 doc: list support for BIP 339 in doc/bips.md (Pieter Wuille) Pull request description: This is based on bitcoin#18044 (comment). A new type `GenTxid` is added to protocol.h, which represents a tagged txid-or-wtxid. The tx request logic is updated to use these instead of uint256s, permitting per-announcement distinguishing of txid/wtxid (instead of assuming that everything we want to request from a wtxid peer is wtx). Then the restriction of orphan-parent requesting to non-wtxid peers is lifted. Also document BIP339 in doc/bips.md. ACKs for top commit: jnewbery: Code review ACK 10b7a6d jonatack: ACK 10b7a6d ajtowns: ACK 10b7a6d -- code review. Using gtxid to replace the is_txid_or_wtxid flag for the mempool functions is nice. naumenkogs: utACK 10b7a6d Tree-SHA512: d518d13ffd71f8d2b3c175dc905362a7259689e6022a97a0b4f14f1f9fdd87475cf5af70cb12338d1e5d31b52c12e4faaea436114056a2ae9669cb506240758b

d4a1ee8 Further improve comments around recentRejects (Suhas Daftuar) f082a13 Disconnect peers sending wtxidrelay message after VERACK (Suhas Daftuar) 22effa5 test: Use wtxid relay generally in functional tests (Fabian Jahr) e481681 test: Add tests for wtxid tx relay in segwit test (Fabian Jahr) 6be398b test: Update test framework p2p protocol version to 70016 (Fabian Jahr) e364b2a Rename AddInventoryKnown() to AddKnownTx() (Suhas Daftuar) 879a3cf Make TX_WITNESS_STRIPPED its own rejection reason (Suhas Daftuar) c1d6a10 Delay getdata requests from peers using txid-based relay (Suhas Daftuar) 181ffad Add p2p message "wtxidrelay" (Suhas Daftuar) 9382672 ignore non-wtxidrelay compliant invs (Anthony Towns) 2599277 Add support for tx-relay via wtxid (Suhas Daftuar) be1b7a8 Add wtxids to recentRejects (Suhas Daftuar) 7384521 Add wtxids of confirmed transactions to bloom filter (Suhas Daftuar) 606755b Add wtxid-index to orphan map (Suhas Daftuar) 3654937 Add a wtxid-index to mapRelay (Suhas Daftuar) f7833b5 Just pass a hash to AddInventoryKnown (Suhas Daftuar) 4df3d13 Add a wtxid-index to the mempool (Suhas Daftuar) Pull request description: 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. ACKs for top commit: fjahr: re-ACK d4a1ee8 instagibbs: reACK d4a1ee8 laanwj: re-ACK d4a1ee8 hebasto: re-ACK d4a1ee8, only rebased since my [previous](#19606 (review)) review: Tree-SHA512: 1bb8725dd2313a9a03cacf8fb7317986eed3d8d1648fa627528441256c37c793bb0fae6c8c139d05ac45d0c7a86265792834e8e09cbf45286426ca6544c10cd5

jnewbery · 2020-11-05T15:40:13Z

Backported to 0.20 in #20317

fanquake added the P2P label Jul 23, 2020

fanquake requested review from sdaftuar and ajtowns July 23, 2020 01:01

sipa force-pushed the 202007_wtxid_followup branch from 7189ef5 to 154778d Compare July 23, 2020 02:50

naumenkogs reviewed Jul 23, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

ajtowns approved these changes Jul 23, 2020

View reviewed changes

src/primitives/transaction.h Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

jonatack reviewed Jul 23, 2020

View reviewed changes

doc/bips.md Outdated Show resolved Hide resolved

sipa force-pushed the 202007_wtxid_followup branch 2 times, most recently from 968edda to 338f80e Compare July 23, 2020 23:35

ariard reviewed Jul 24, 2020

View reviewed changes

sdaftuar reviewed Jul 24, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

sipa force-pushed the 202007_wtxid_followup branch from 338f80e to 707d0f4 Compare July 25, 2020 21:29

jonatack reviewed Jul 26, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Jul 26, 2020

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

naumenkogs reviewed Jul 27, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 27, 2020

Deduplicate parent txid loop of requested transactions and missing parents of orphan transactions #19596

Merged

This was referenced Jul 27, 2020

p2p, refactor: add CInv transaction message helpers; use in net processing #19590

Merged

Remove mempool global #19556

Merged

jnewbery reviewed Jul 29, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

This was referenced Jul 29, 2020

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

[RFC] Package-relay: sender-initiated #19621

Closed

Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766

Merged

jnewbery reviewed Jul 30, 2020

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

DrahtBot added the Needs rebase label Jul 30, 2020

sipa and others added 7 commits July 30, 2020 12:39

doc: list support for BIP 339 in doc/bips.md

d362f19

refactor: add GenTxid (=txid or wtxid) type and use it for tx request…

9efd86a

… logic

p2p: enable fetching of orphans from wtxid peers

900d7f6

Based on a commit by Anthony Towns.

test: request parents of orphan from wtxid relay peer

e65d115

refactor: use GenTxid in tx request functions

a2bfac8

refactor: make FindTxForGetData use GenTxid

5c124e1

refactor: make txmempool interface use GenTxid

10b7a6d

sipa force-pushed the 202007_wtxid_followup branch from c048357 to 10b7a6d Compare July 30, 2020 20:46

DrahtBot removed the Needs rebase label Jul 30, 2020

DrahtBot mentioned this pull request Jul 31, 2020

Implement ADDRv2 support (part of BIP155) #19031

Closed

11 tasks

fanquake merged commit f7c73b0 into bitcoin:master Jul 31, 2020

sipa mentioned this pull request Sep 29, 2020

Overhaul transaction request logic #19988

Merged

jnewbery mentioned this pull request Nov 5, 2020

Backport wtxid orphan fetch to v0.20 #20317

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Enable fetching of orphan parents from wtxid peers #19569

Enable fetching of orphan parents from wtxid peers #19569

Uh oh!

Conversation

sipa commented Jul 23, 2020

Uh oh!

sipa commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Jul 23, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Jul 24, 2020

Uh oh!

Uh oh!

ariard Jul 24, 2020

Choose a reason for hiding this comment

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sipa commented Jul 25, 2020

Uh oh!

jnewbery commented Jul 26, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sipa commented Jul 26, 2020

Uh oh!

sdaftuar commented Jul 26, 2020

Uh oh!

sdaftuar commented Jul 26, 2020

Uh oh!

Uh oh!

naumenkogs commented Jul 27, 2020

Uh oh!

DrahtBot commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

sipa commented Jul 27, 2020

Uh oh!

sdaftuar commented Jul 27, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jul 30, 2020

Uh oh!

Uh oh!

sipa commented Jul 30, 2020

Uh oh!

ajtowns commented Jul 31, 2020

Uh oh!

naumenkogs commented Jul 31, 2020

Uh oh!

jonatack commented Jul 31, 2020

Uh oh!

jnewbery commented Jul 31, 2020

Uh oh!

jnewbery commented Jul 31, 2020

sipa commented Jul 23, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading

DrahtBot commented Jul 27, 2020 •

edited

Loading