Package Relay and child-with-unconfirmed-parents + tx-with-unconfirmed-ancestors Packages

glozow commented at 3:26 pm on May 17, 2022: member

Please leave concept/approach feedback on the mailing list thread so that people can follow the discussion in one place. Feel free to leave a review comment if you find typos or incorrect wording.

in bip-v1-packages.mediawiki:180 in dee1711789 outdated

158+
159+A '''child-with-unconfirmed-parents''' package MUST be:
160+
161+# '''Sorted topologically.''' For every transaction t in the package, if any of t's parents are present in the package, the parent must appear somewhere in the list before t. In other words, the transactions must be sorted in ascending order of number of ancestors present in the package.
162+
163+# '''Only 1 child with unconfirmed parents.''' The package must consist of one transaction and its unconfirmed parents. There must not be any other transactions in the package. Other dependency relationships may exist within the package (e.g. one parent may spend the output of another parent) provided that topological order is respected.

sdaftuar commented at 4:40 pm on May 17, 2022:

One thing I overlooked when I first read this over – by “unconfirmed parents” you mean direct parents, ie transactions which contain an outpoint being spent in the child transaction, is that right? (I was trying to figure out exactly what you meant by the 2-generation limit further down, and then I realized I might have misunderstood the language here.)

I’d suggest defining this language more explicitly somehow, since I think “unconfirmed parent” can read a lot like “unconfirmed ancestor” (and at any rate I don’t think these words have meanings well defined across the developer community).

Also, if a GETDATA(PCKG1, wtxid) comes in for a transaction that has unconfirmed ancestors outside the set of unconfirmed direct parents, how is software supposed to respond?

glozow commented at 5:58 pm on May 17, 2022:

by “unconfirmed parents” you mean direct parents, ie transactions which contain an outpoint being spent in the child transaction, is that right?

Yes, exactly.

I’d suggest defining this language more explicitly somehow, since I think “unconfirmed parent” can read a lot like “unconfirmed ancestor” (and at any rate I don’t think these words have meanings well defined across the developer community).

Ah, noted. How about something like this: “Given any two transactions Tx0 and Tx1 where Tx1 spends an output of Tx0, Tx0 is a parent of Tx1 and Tx1 is a child of Tx0. A transaction’s ancestors include, recursively, its parents, the parents of its parents, etc. A transaction’s descendants include, recursively, its children, the children of its children, etc.”

Also, if a GETDATA(PCKG1, wtxid) comes in for a transaction that has unconfirmed ancestors outside the set of unconfirmed direct parents, how is software supposed to respond?

It should send only the direct parents, and no other ancestors.

in bip-v1-packages.mediawiki:265 in dee1711789 outdated

220+A client using BIP330 reconciliation-based transaction relay (Erlay) is able to use package relay
221+without interference. In fact, a package of transactions may be announced using both Erlay and
222+package relay.  After reconciliation, if the initiator would have announced a transaction by wtxid
223+but also has package information for it, they may send "inv(MSG_PCKG)" instead of "inv(WTX)".
224+
225+[[File:./bip-v1-packages/package_erlay.png|900px]]

sdaftuar commented at 4:44 pm on May 17, 2022:

In the erlay picture, are some of the arrows at the bottom of the image needing to be reversed?

glozow commented at 2:40 pm on May 18, 2022:

Thanks! fixed

in bip-package-relay.mediawiki:47 in dee1711789 outdated

42+
43+# Receiver requests package information.
44+
45+# The sender provides package information, including the wtxids of the transactions in the package and anything else that might be relevant (e.g. total fees and size).
46+
47+# The reciever uses the package information to decide how to request and validate the transactions.

sdaftuar commented at 4:45 pm on May 17, 2022:

“receiver” (here and at line 59)

in bip-v1-packages.mediawiki:137 in dee1711789 outdated

132+
133+# Upon receipt of a "pckginfo1" message, the node should decide if it wants to validate the package, request transaction data if necessary, etc.
134+
135+# Upon receipt of a malformed "pckginfo1" message or package that does not abide by the max_count, max_weight, or other rules specified by the version agreed upon in the initial negotiation, the sender should be disconnected.  If a node receives a "pckginfo1" message for which the "pckg_fee" or "pckg_weight" do not reflect the true total fees and weight, respectively, or the transactions in the package, the message is malformed.
136+
137+# A node MUST NOT send a "pckginfo1" message that has not been requested by the recipient. Upon receipt of an unsolicited "pckginfo1", a node should disconnect the sender.

sdaftuar commented at 4:53 pm on May 17, 2022:

Requested by recipient means a GETDATA(MSG_PCKG1) was sent? If so I’d suggest making that more explicit.

Also, I think usually we have left the handling of protocol violations as not themselves mandating particular handling, ie, I’d suggest that the spec be “Upon receipt of an unsolicited pckginfo1, a node MAY disconnect the sender”.

glozow commented at 2:41 pm on May 18, 2022:

Added, thanks!

in bip-v1-packages.mediawiki:158 in dee1711789 outdated

140+
141+====MSG_PCKG1====
142+
143+# A new inv type (MSG_PCKG1 == 0x6) is added, for use in inv messages and getdata requests pertaining to version 1 packages.
144+
145+# As an inv type, it indicates that both transaction data and version 1 package information are available for the transaction. The transaction is referenced by its wtxid. As a getdata request type, it indicates that the sender wants package information for the transaction.

sdaftuar commented at 4:53 pm on May 17, 2022:

Does using MSG_PCKG1 as an inv-type imply that the transaction’s unconfirmed ancestors are exactly equal to its unconfirmed parents? (See my other questions below)

in bip-v1-packages.mediawiki:166 in dee1711789 outdated

142+
143+# A new inv type (MSG_PCKG1 == 0x6) is added, for use in inv messages and getdata requests pertaining to version 1 packages.
144+
145+# As an inv type, it indicates that both transaction data and version 1 package information are available for the transaction. The transaction is referenced by its wtxid. As a getdata request type, it indicates that the sender wants package information for the transaction.
146+
147+# Upon receipt of a "getdata" request for "MSG_PCKG1", the node should respond with the version 1 package corresponding to the requested transaction and its current chain tip, or with NOTFOUND. The node should not assume that the sender is requesting the transaction data as well.

sdaftuar commented at 4:54 pm on May 17, 2022:

Is it permitted to send a GETDATA(MSG_PCKG1, wtxid) if the peer didn’t send an INV(MSG_PCKG1, wtxid)? Ie could software use this message to reconstruct the parents of an orphan transaction?

glozow commented at 8:18 pm on May 17, 2022:

Yes absolutely! This kind of “half solves” the orphan fetching use case if there’s only direct parents missing, but I actually think it would make sense to create another type of package for tx-with-unconfirmed-ancestors:

To answer the likely question of “why wasn’t v1 packages tx-with-unconfirmed-ancestors?” I have two primary reasons:

The mempool policy created for child-with-unconfirmed-parents packages is insufficient to ensure that a the fees of a tx-with-ancestors package would be evaluated correctly. I noticed you had some consideration for that in #16401. Probably the best algorithm is to sort topologically, then submit each transaction with its ancestor subset within the package. But it doesn’t seem like anyone is asking for more than 2 generations of fee-bumping, and I haven’t really figured out what RBF would look like.
It seems like the orphan-processing and fee-bumping use cases can be thought of separately. For instance, fee-bumping should be a sender-initiated “I’m telling you about extra info you need” and orphan-fetching is more appropriate as a receiver-initiated “hey I’m new here, so I don’t know the ancestors of this transaction.”

sdaftuar commented at 4:57 pm on May 17, 2022: member

Thanks for working on this! Just some initial questions to try to make sure I’m understanding this proposal right (will do my best to save broader feedback for the mailing list!).

glozow force-pushed on May 17, 2022

instagibbs commented at 8:53 pm on May 17, 2022: member

Based on email discussion, I’d request that the spec makes it explicit that you do not request packages from peers when they report a different chaintip blockhash(and disconnect them right after…)

it’s possible I missed it, but I keep looking and not seeing it

glozow commented at 9:01 pm on May 17, 2022: member

Based on email discussion, I’d request that the spec makes it explicit that you do not request packages from peers when they report a different chaintip blockhash(and disconnect them right after…)

Thanks @instagibbs I will add this. Also, if a peer sends us a “pckginfo1” with a blockhash that’s different from ours, we should probably just drop it and re-request when they catch up (I think nowadays we usually have an accurate idea of what our peer’s best block is). We’re definitely not going to disconnect a peer for sending a “pckginfo1” with a different blockhash from ours.

glozow force-pushed on May 17, 2022

in bip-package-relay.mediawiki:33 in 66b050af1a outdated

28+
29+Two main ideas are introduced:
30+
31+# Download and validate packages of transactions together.
32+
33+# Provide information to help peers decide whether to request and/or how validate transactions which are part of a package.

michaelfolkson commented at 12:37 pm on May 19, 2022:

nit: s/how validate/how to validate

in bip-package-relay.mediawiki:214 in 66b050af1a outdated

211+# When package mempool acceptance is upgraded to support more types of packages, increment the version number (similar to Erlay). During version handshake, peers negotiate which version of package relay they will use by each sending one "sendpackages" message.
212+
213+# When introducing another type of package, assign a version number to it and announce it as an additional supported version (similar to Compact Block Relay). During version handshake, peers send one "sendpackages" message for each version supported.
214+
215+The second option was favored because it allows different parameters for different versions.
216+For example, it should be possible to allow "arbitrary topology but maximum 3-transaction" package

michaelfolkson commented at 12:38 pm on May 19, 2022:

nit: s/3-transaction package/3-transaction packages

glozow commented at 6:31 pm on May 24, 2022:

Text no longer there

in bip-package-relay.mediawiki:159 in 66b050af1a outdated

162+alternative solutions based on the resources used to communicate (not necessarily trustworthy)
163+information: We would like to minimize network bandwidth, avoid downloading a transaction more than
164+once, avoid downloading transactions that are eventually rejected, and minimize storage allocated
165+for not-yet-validated transactions.
166+
167+Consider these scenarios specifically transaction relay:

michaelfolkson commented at 12:41 pm on May 19, 2022:

nit: s/specifically transaction relay/specifically for transaction relay

in bip-package-relay.mediawiki:162 in 66b050af1a outdated

157+==Rationale==
158+
159+===P2P Message Design===
160+
161+These p2p messages are added for communication efficiency and, as such, one should measure
162+alternative solutions based on the resources used to communicate (not necessarily trustworthy)

michaelfolkson commented at 12:42 pm on May 19, 2022:

nit: s/not necessarily trustworthy/in the presence of possibly malicious peers

in bip-package-relay.mediawiki:95 in 66b050af1a outdated

92+|-
93+|}
94+
95+# The "sendpackages" message has the structure defined above, with pchCommand == "sendpackages".
96+
97+# During version handshake, nodes should send a "sendpackages" message indicate they support package relay and may request packages.

michaelfolkson commented at 12:51 pm on May 19, 2022:

nits: s/During version handshake/During the version handshake, s/message indicate/ message to indicate

in bip-package-relay.mediawiki:53 in 66b050af1a outdated

48+
49+[[File:./bip-package-relay/receiver_init_dialogue.png|600px]]
50+
51+''Diagram: A receiver-initiated dialogue.''
52+
53+Sometimes, no matter what order transactions are received by a node, validating them individually is

michaelfolkson commented at 12:56 pm on May 19, 2022:

nit: s/no matter what order transactions are received by a node/regardless of the order in which transactions are received by a node

in bip-package-relay.mediawiki:124 in 66b050af1a outdated

120+
121+# A "getpckgtxns" message should be used to request all or some of the transactions previously announced in a "pckginfo" message, specified by witness transactiosome id.
122+
123+# Upon receipt of a "getpckgtxns" message, a node must respond with either a "pckgtxns" containing the requested transactions or a "notfound" message indicating one or more of the transactions is unavailable. This allows the receiver to avoid downloading and storing transactions that cannot be validated immediately.
124+
125+# A "getpckgtxns" message should only be sent if both peers agreed to send packages in the version handshake. If a "getpckgtxns" message is received from a peer with which package relay was not negotiated, the sender should be disconnected.

michaelfolkson commented at 1:04 pm on May 19, 2022:

Trying to think if disconnect is the right response here rather than merely ignore and not disconnect. Sending it (when no negotiation has taken place) does seem to suggest at the very least that the P2P logic of the node is faulty which I guess is reason to disconnect.

glozow commented at 6:30 pm on May 24, 2022:

Thanks, this has been changed so the comment no longer applies

michaelfolkson commented at 1:19 pm on May 19, 2022: contributor

First read through. Looks great thus far. Just some nits (feel free to ignore).

I know this hasn’t been done in the past but in the spirit of trying to make BIPs more understandable (and precise when it comes to references to particular attacks) I wonder if a glossary of terms (e.g. pinning attacks and the different varieties of such attacks) could be a good idea (maybe footnotes rather than a glossary, they have been used in the past). I guess the top priority is to clearly go into what attacks/vulns package relay addresses (including on higher layers), what attacks/vulns package relay potentially opens up on the base layer with their introduction and how the design of package relay has minimized the scope of those attacks/vulns on the base layer. My gut feel is that the current draft is still a little light on these details but it was just a first readthrough and it clearly doesn’t ignore them.

edit: Was looking at the BIP118 draft for inspiration on what a BIP should cover that is primarily targeted towards higher layer use cases (but with base layer implications). That has a Security section which may be a good idea for this BIP.

glozow force-pushed on May 20, 2022

glozow renamed this:
~~Package Relay and child-with-unconfirmed-parents Packages~~
Package Relay and child-with-unconfirmed-parents + tx-with-unconfirmed-ancestors Packages
on May 20, 2022

glozow commented at 10:57 pm on May 20, 2022: member

Added tx-with-unconfirmed-ancestors packages as a second type of package dedicated to orphan-fetching.

glozow force-pushed on May 24, 2022

glozow commented at 6:28 pm on May 24, 2022: member

Last push (thanks @ajtowns)

Count and size are implied by the version. Version 1 and Version 2 both have maximum of 25 transactions and 404KWu.
Announce sendpackages based on our own state. It’s ok to send “sendpackages” if they sent fRelay=false.
At verack, require fRelay=true and wtxidrelay if they sent sendpackages, otherwise disconnect.
If we get “getpckgtxns” or “pckgtxns” without having negotiated “sendpackages” ahead of time, ignore, don’t disconnect. Emphasize that the intention is to validate all of the transactions received through “pckgtxns” together.

Thanks for the feedback @michaelfolkson, I’ll get to the style/wording comments later when the design is more finalized.

flack commented at 5:38 pm on May 25, 2022: contributor

Sorry for also leaving a style nit, but as someone also mentioned on the mailing list, pkg is a much more common abbreviation than pckg (it’s even mentioned on Wikipedia: https://en.wikipedia.org/wiki/PKG), so it might be nice to consider switching to that.

specify generic package relay 09cfd6eae2

specify version 1 child-with-unconfirmed-parents package relay 2c53cddbc1

specify version 2 transaction-with-unconfirmed-ancestors package relay 9a85feff61

glozow force-pushed on Jun 7, 2022

glozow commented at 5:47 pm on June 7, 2022: member

Last push:

Renamed s/pckg/pkg everywhere, updated diagrams
Removed fee and weight information from pkginfo1

in bip-v1-packages.mediawiki:174 in 2c53cddbc1 outdated

169+sent between nodes MUST abide by the rules below, otherwise the package is malformed and the sender
170+should be disconnected.
171+
172+A version 1 or '''child-with-unconfirmed-parents''' package can be defined for any transaction that spends
173+unconfirmed inputs. The child can be thought of as the "representative" of the package. This package
174+can be uniquely identified by the transaction's wtxid and the current chain tip block hash.

sdaftuar commented at 4:31 pm on June 8, 2022:

The child’s wtxid does not commit to the wtxids of its ancestors (just the txids), so I don’t think it’s correct to say that a package is uniquely identified by the child transaction’s wtxid and the current chain tip. (I tried to bring this point up in the mailing list thread as well, in the context of package de-duplication.)

glozow commented at 8:50 am on June 16, 2022:

Yes sorry, I think I should have said the package can be easily reconstructed using the transaction’s wtxid and current chain tip block hash.

in bip-package-relay.mediawiki:11 in 9a85feff61

 6+  Comments-Summary:
 7+  Comments-URI:
 8+  Status: Draft
 9+  Type: Standards Track
10+  Created: 2022-04-14
11+  License: PD

luke-jr commented at 9:33 pm on July 25, 2022:

Before taking this out of draft, please license it under one of the acceptable licenses

glozow commented at 12:45 pm on July 26, 2022:

Thanks will do! I’m still working on a few changes based on feedback, and then will take this out of draft.

glozow commented at 9:37 pm on October 13, 2022: member

Closing as the updated proposal is going to be significantly different from this one, which has no number assigned, and I think it would be confusing.

glozow closed this on Oct 13, 2022

glozow cross-referenced this on Oct 13, 2022 from issue BIP 331: Ancestor Package Relay by glozow

chinggg cross-referenced this on Dec 6, 2022 from issue fuzz: Modify tx_pool_standard target to test package processing by chinggg

Package Relay and child-with-unconfirmed-parents + tx-with-unconfirmed-ancestors Packages #1324