Package Relay Draft 1 (Single In-Flight Approach)

[glozow]

Experimental code.

General structure
This might be helpful for reviewing. The PR can also be split into sub-PRs like this.

0. Mempool logic to make sure we can handle whatever the peer throws at us (just rebased on here, can work on these in parallel). This PR is open, n26711.

   (P2P stuff starts at commit 52bf734)

1. Refactoring, create a TxPackageTracker module that always exists even if package relay is turned off. It wraps the TxOrphanage and forwards calls.

2. Beef up an "orphan resolution module" within TxPackageTracker. It keeps track of what orphans we're trying to get parents for (currently still requesting by txid) and tells Peerman what should be downloaded/validated, when. Improve orphan handling by remembering which peers announced the orphan to allow retrying failed requests, requesting from preferred first, accounting for overloaded peers, etc. Tweak the eviction/rate limiting logic so that, even if an inbound is sending lots of orphans, we're still able to resolve orphans with outbounds at 1/2*INVENTORY_BROADCAST_PER_SECOND rate (for example).

3. Add a -packagerelay option and the "sendpackages" negotiation logic.
   Add logic for requesting MSG_ANCPKGINFO and serving "ancpkginfo." When resolving orphans, prefer package relay peers and ask for "ancpkginfo" from them.

4. When receiving package info, track which transactions need to be downloaded. At this point, just download and validate individually. Protect stuff that's already in our orphanage from eviction. (Note: in the future, we can add new package info versions, while keeping the PackageToDownload data structures generic).
   Add "getpkgtxns" and "pkgtxns" logic, and `MSG_PKGTXNS. Download and submit packages.

Todos

• Have TxPackageTracker wrap/own TxOrphanage
• Protect some quantity of orphans, ensure that we can always get packages relayed from outbounds
• Tweak delays / expiry to make sure 1 peer can't keep stalling to make us forget tx
• Needs unit tests and fuzz for TxPackageTracker
• Cache rejections of subpackages

Done but in other branches

• When Loading mempool from disk, ensure packages persist

Additional Maybe Todos

• Look in vExtra for parents/transactions listed in ancpkginfo
• if txn-already-have (recently confirmed), Exclude()
• Add a map of <wtxid, {fees, size}> to MemPoolAccept to check ancestor subpackage fees prior to sending to AcceptMultipleTransactions
• fuzz Packageifier
• If orphanage is full, fall back to downloading another time

[mzumsande]

also shouldn't send SENDPACKAGES for block-relay-only connections (check GetTxRelay()).

[Yugthakkar04]

        m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::WTXIDRELAY));

this package would cost a block relay function to optimize it and render it. there should not bee the GetTxRelay()
function it would block the preview.
sets and manages terminal lines and ports

[dergoegge]

GetTime is deprecated

[dergoegge]

all the members are missing the m_ prefix

[glozow]

Some notes on m_recent_rejects vs m_recent_rejects_reconsiderable.

The goals are:

• Don't repeat work, i.e. don't download or validate transactions more than once if we can know they're going to be rejected.
• Don't allow censorship due to overzealous rejection caching.

m_recent_rejects stores the wtxids of transactions we will not download/validate by themselves again until the chain tip changes. Even if we see them in a package, we'll reject them. This includes failures for consensus, and also some policy like tx-too-big.

m_recent_rejects_reconsiderable contains wtxids of individual transactions and packages that we will not download/validate by themselves, but will reconsider if they're part of a package. This includes transactions and packages that failed for being too low feerate. It can also include orphans and transactions with ephemeral anchor outputs where we haven't seen the child yet. The idea is that we'll reconsider them with another tx that makes them valid (e.g. high-fee child), but not otherwise.

Upon receiving an inv for 1 transaction, reject if it is in m_recent_rejects or m_recent_rejects_reconsiderable.

Upon receiving an ancpkginfo:

• If any of the transactions are in m_recent_rejects, reject this package and don't download the txdata. However, it is ok to continue asking other peers for the ancpkginfo in case somebody else provides a different list.
• If the entire package is in m_recent_rejects_reconsiderable, reject this package and don't download the txdata (it will just be duplicate work). However, it is ok to continue asking other peers for the ancpkginfo in case somebody else provides a different list.

Here is an example:

Let's take this order of events:

1. Downloaded orphan F. Resolved by downloading ancpkginfo {ABCDEF}
   A: submit, {AC}
   B: submit, B
   C: submit, {AC}
   D: reject
   E: reject
   F: reject
   recent_rejects: D
   reconsiderable: E, F, {ABCDEF}
   You continue trying to resolve F. If another peer says the ancpkginfo is ABCDEF, there is no need to download/validate again.

2. Downloaded orphan G. Resolved by downloading ancpkginfo {EG}. E was rejected but is in the reconsiderable recent rejects, so you request the tx data and submit as a package.
   E: reject
   G: reject
   reconsiderable: E, G, {EG}
   You continue trying to resolve G. If another peer says the ancpkginfo is EG, there is no need to download/validate again.

3. Downloaded orphan H. Resolved by downloading ancpkginfo {EGH}. E and G and {EG} were rejected but are in the reconsiderable recent rejects, so you request the tx data and submit as a package.
   E: submit, {EGH}
   F: submit, {EGH}
   H: submit, {EGH}

4. Downloaded orphan J. Resolved by downloading ancpkginfo {ABCDEFJ}.
   You reject this package without downloading, because D had an invalid signature. However, you continue asking other peers for ancpkginfo in case the first peer lied to you. If they also say the ancpkginfo is {ABCDEFJ}, you don't download the tx data.

[dergoegge]

Maybe add these new methods to fuzz/txorphan.cpp

[dergoegge]

should wrap all of this in namespace node.

[dergoegge]

this LogPrintf looks like a disk filler

[dergoegge]

would move the max size check up to the empty check

[dergoegge]

Suggested change

	std::optional<std::vector<uint256>> PeerManagerImpl::MaybeGetAncPkgInfo(Peer& peer, const CTransactionRef& tx)
	{
	if (!peer.m_package_relay) {
	return std::nullopt;
	}
	std::vector<uint256> PeerManagerImpl::MaybeGetAncPkgInfo(const CTransactionRef& tx)
	{

Check this at the call site of MaybeGetAncPkgInfo instead. Simplifies the function signature and you already have an if statement there for this anyway.

[dergoegge]

No size limit? Maybe do something similar to what we do when receiving headers (i.e. peek at the size before de-serializing)

[dergoegge]

No size limit?

[dergoegge]

Suggested change

	m_txpackagetracker = std::make_unique<TxPackageTracker>(m_orphanage);
	if (m_enable_package_relay) m_txpackagetracker = std::make_unique<TxPackageTracker>(m_orphanage);

Or if the package tracker should always exist, make it a non-unique_ptr member of PeerManagerImpl. Then you can also remove all the existence checks along the way.

[jnewbery]

These two members are only used when they're written in ReceivedVerack() and then immediately read back. Can you remove them and have the CanRelayPackages() logic directly in ReceivedVerack()?

[jnewbery]

Can we remove this bool and replace it with !m_versions_in_common.empty()? Do we care if the peer has only sent us sendpackages for versions we don't understand?

[jnewbery]

Do we need this function? Can we just add the node to registration_states in ReceivedSendpackages() if it isn't there already?

[glozow]

Yeah, tbh it seems like I overcomplicated this and could just have 1 function to add a peer after verack

[jnewbery]

Can we make this const and set it in the constructor?

[jnewbery]

Perhaps:

Suggested change

	mutable Mutex m_mutex;
	mutable Mutex m_package_tracker_mutex;

to help with grepping.

[jnewbery]

Can this (and Peer:m_package_relay) be removed and we just query m_txpackagetracker for whether a peer supports package relay and how many peers we have that support package relay?

[ariard]

Reviewed up to “[validation] set PackageValidationState when mempool full”.

[ariard]

I think this new “CPFP within a package” ability might alter pinning considerations on legacy channels (i.e before option_anchor_outputs is still the default on CLN iirc and at least on LDK).

The to_remote output format on legacy commitment transaction is a P2WPKH. There is no 1 CSV as with anchor output. As soon as the local commitment transaction is broadcast on the transaction-relay network, the counterparty can attach a CPFP to amplify the propagation probabilities of the local commitment transaction. This CPFP attachment might be not be seen by the broadcaster of the local commitment transaction. This CPFP can be used as a pinning child vector (e.g rule 3 on absolute fee). The broadcast of a local commitment transaction can be be trigger by a malicious counterparty by refusing an update_fee even in the lack of pending HTLC outputs.

So from my understanding the deployment of a “CPFP within a package” might enable some new pinning scenarios against legacy channels (even when there is no pending HTLC outputs) ? I think in term of severity those scenarios are equivalent where the pinnings are trigger by a counterparty and there are pending HTLC outputs. Note there is no currently standardized or deplyed channel upgrade mechanism to uplift legacy channel types to newer anchor output.

Independently of pinnings, I wonder if those low-feerate commitment transactions carried on by a “CPFP within a package” could be leveraged to open the way to some miner fee-harvesting attacks ?

We might have to swallow the bullet after all. Though if we have do it, I prefer to do it in knowledge of the opening attack surface, at least if we can land some easy mitigations on the Lightning-side.

[ariard]

What is the state of the discussion about modifying or removing -minrelaytxfee ? I think there was some discussions in the past as tracked by optech https://bitcoinops.org/en/topics/default-minimum-transaction-relay-feerates/

From my understanding, minrelaytxfee constitutes a strong transaction-relay propagation liability for contract protocols as mentioned in mempool-limits.md. I think if you have a pre-signed 1sat/vbyte LN commitment transaction or a vault withdrawal transaction 5sat/vbyte and there is a major increase in the minrelaytxfee to 10sat/vbyte all your transactions won’t propagate, and this whatever the CPFP feerate iiuc proposed changes.

With current -minrelaytxfee of 1 sat/vbyte (DEFAULT_INCREMENTAL_RELAY_FEE) this not a concrete propagation risk for contract protocol. However this setting might be changed at the initiative of node operators or by the project itself in the future. From my understanding, this would put contract protocol node operators in a difficult position as LN counterparty cooperation cannot be assumed to re-sign new transactions to a higher feerate or vault key ceremony coordinated before there is a deployment of such “bumped” -minrelaytxfee.

So I’m thinking if a “wildcard” could be granted to nversion=3 transactions where the parent transaction can be any feeerate and this compensated by the CPFP ?

[glozow]

I don't think we are planning to remove -minrelaytxfee entirely.

Yes, the plan is to allow v3 transactions to be 0 fee when they are cpfp'd

[ariard]

What is the pruning strategy in the hypothesis of disconnected blocks where low-feerate package transactions are resuscitated back in the network mempools and the CPFP replaced following this disconnection by a better block mining a partial double-spend of the CPFP but not the parent themselves ?

[ariard]

I think it’s a bit confusing that a PackageValidationResult’s reject_reason says transaction failed. A more indicative error message could be whole package submission failed - individual transaction mempool inclusion failed. It’s more verbose though at least make easier to debug that the package integrity has been respected by our mempool logic.

[ariard]

Reviewed until “[mempool] evict everything below min relay fee in TrimToSize()"

[ariard]

Note IsPackageWellFormed is called twice for the same set of transaction, once L1349 and once L1249. From my understanding of the checks there, and with the fact that only a diminished of transactions is given the second time, the checks should always yield valid. A bypass argument could be given to AcceptSingleTransaction to gain a bit in performance.

A more fundamental simplication could be to unify submitpackage and testmempoolaccept. I don’t know if we have gathered users feedback on the relevance of multi-transactions testmempoolaccept since it’s available. At the very least the difference in code paths and as such of checks can be a source of confusion as much for developers than for users on what properties are enforced.

[ariard]

I think this creates a non-incentive-compatible optimal blind spot where a transaction feerate could be under m_min_relay_fee though with a non-zero absolute fee and network mempools being empty of better blockspace demand. All that said, I’m not sure such transactions can be issued by robust broadcast clients respecting m_min_relay_feerate, there is still the possibility of wrongly implemented clients or relying on third-party fee-estimators for such checks ? I don’t see other risks.

[ariard]

From my understanding, if you shutdown at block H with mempool size S and dump mempool file M, then if you re-load mempool file M at same block H and with same configuration of mempool size S, the whole content of the file should be loaded in memory with no transaction marked as evicted ? If we would like to improve error detection, like a mempool file corrupted, maybe we can “watermark” the mempool file with some information like block height, mempool size at time of dump, configured mempool min fees…if it’s not already the case ? I’m not sure if error detection of mempool dump is that important, though it might be annoying for a miner to have a stale mempool view.

[ariard]

Can say there is no max bound enforce and “sort” verification can happen on any size and depth of package?