Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package Relay Draft 1 (Single In-Flight Approach) #8

Closed
wants to merge 57 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
43fd360
-- Part 0: Mempool Logic for Arbitrary Packages--
glozow Feb 20, 2023
f154c34
[test util] mock mempool minimum feerate
glozow Jan 17, 2023
5d09d97
[test] package cpfp bumps parents <mempoolminfee but >=minrelaytxfee
glozow Jan 17, 2023
64fc0da
[policy] disallow transactions under min relay fee, even in packages
glozow Jan 17, 2023
1f40f8c
[validation] set PackageValidationState when mempool full
glozow Jan 24, 2023
3639ad0
[validation] call AcceptSingleTransaction when only 1 package tx left
glozow Dec 15, 2022
a781945
[mempool] evict everything below min relay fee in TrimToSize()
glozow Jan 17, 2023
daf6b81
[test framework] return txhex from create_lots_of_big_transactions
glozow Jan 19, 2023
569c284
[test] raise wallet_abandonconflict -minrelaytxfee settings
glozow Jan 19, 2023
faf8b23
[mempool] persist packages across restart
glozow Jan 19, 2023
f9b1993
MOVEONLY: move package checks into helper functions
glozow Sep 16, 2022
2d0d4c0
scripted-diff: rename CheckPackage to IsPackageWellFormed
glozow Jan 19, 2023
9bfb4a1
[packages] Packageifier for arbitrary transaction lists
glozow Sep 16, 2022
87e43a5
[test util] multi-input multi-output CreateValidMempoolTransaction
glozow Oct 3, 2022
2109d36
[validation] pre-fill missing inputs for txns depending on invalid
glozow Dec 14, 2022
9bcefd6
[validation] validate packages by submitting each tx's ancestor sub-p…
glozow Dec 15, 2022
0c011bc
[policy] allow any ancestor package, not just child-with-unconfirmed-…
glozow Feb 17, 2023
e42d8d9
-- Part 1: Orphan Resolution Module --
glozow Feb 20, 2023
0821236
[p2p] add tx package tracker
glozow May 1, 2022
bcb33bb
[refactor] wrap TxOrphanage inside TxPackageTracker
glozow Oct 6, 2022
718ce4a
[txorphange] GetTx by wtxid
glozow Jan 24, 2023
60566f4
[txorphanage] EraseTx by wtxid
glozow Feb 20, 2023
dce6d8b
[txorphanage] return erased wtxids from EraseForBlock
glozow Feb 20, 2023
2e72630
[log] add category TXPACKAGES for orphanage and package relay
glozow Mar 6, 2023
49aa386
[txorphanage] impose a maximum total size of orphans
glozow Mar 31, 2023
9a65983
[txorphanage] track bytes provided per peer
glozow Apr 18, 2023
2861927
[txorphanage] when peers are overloaded, favor their orphans for evic…
glozow Apr 18, 2023
3df9a46
[txorphanage] handle AddTx(nullptr)
glozow Apr 18, 2023
8325214
[txpackagetracker] add orphan resolution tracker
glozow Jan 24, 2023
c24c133
[refactor] use txpackagetracker for orphan resolution
glozow Oct 6, 2022
44c3725
[txpackagetracker] delete unused OrphanageAddTx
glozow Apr 18, 2023
e4d7b44
[txrequest] GetCandidatePeers and ResetRequestTimeout
glozow Mar 20, 2023
b46bafb
[txorphanage] support multiple announcers
glozow Apr 18, 2023
26d7bdc
[unit test] orphanage: multiple announcers, eviction
glozow Apr 18, 2023
6023be6
[p2p] use all orphan announcers as potential parent sources
glozow Oct 6, 2022
f5b00f3
-- Part 2: Signal SENDPACKAGES --
glozow Feb 20, 2023
5c7331b
[txpackagetracker] negotiation logic
glozow Feb 20, 2023
945814c
[unit test] txpackagetracker tests
glozow Feb 20, 2023
3c0e90c
[p2p] signal support for package relay
glozow Oct 7, 2022
424eb15
[rpc] expose package relay on rpc
glozow Sep 6, 2022
25d3fe4
-- Part 3: Request Ancestor Package Info For Orphans --
glozow Feb 20, 2023
797ed82
[packages] hash multiple transactions
glozow Jan 9, 2023
f7d6fea
[validation/p2p] separate TxValidationResult and rejects filter for l…
glozow Sep 12, 2022
8164ad2
[p2p] respond to getdata(ancpkginfo) requests with ancpkginfo
glozow Sep 6, 2022
e6656a9
[txpackagetracker] create ancpkginfo requests using orphan resolution…
glozow Jan 24, 2023
0389892
[unit test] txpackagetracker orphan tracking
glozow Feb 20, 2023
21acc89
[p2p] Resolve orphans by requesting ancpkginfo and requesting txdata
glozow Oct 6, 2022
cc800b3
-- Part 4: Download and Validate Packages --
glozow Feb 20, 2023
ba5e401
[p2p] respond to getpkgtxns with pkgtxns or notfound MSG_PKGTXNS
glozow Oct 3, 2022
852da8e
[txpackagetracker] track packages to download
glozow Jan 5, 2023
fbe58b5
[p2p] use ancpkginfo to make getpkgtxns requests
glozow Jan 6, 2023
8859429
[txpackagetracker] handle pkgtxns/notfound, return validation work items
glozow Jan 9, 2023
252ff5a
[unit test] PkgInfoAllowed, ReceivedAncPkgInfo, ReceivedPkgTxns
glozow Feb 20, 2023
54c4fbe
[p2p] handle pkgtxns and validate packages
glozow Jan 9, 2023
efa53b7
[p2p] don't inv fee-bumping children to non-pkgrelay peers
glozow Apr 4, 2023
d682a10
[rpc] allow submitpackage to be called outside of regtest
glozow Apr 11, 2023
b647166
[functional test] package relay e2e
glozow Oct 3, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions doc/policy/packages.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,24 +80,37 @@ test accepts):
If any transactions in the package are already in the mempool, they are not submitted again
("deduplicated") and are thus excluded from this calculation.

To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate
(`-minrelaytxfee`) and the dynamic mempool minimum feerate, the total package feerate is used instead
of the individual feerate. The individual transactions are allowed to be below the feerate
requirements if the package meets the feerate requirements. For example, the parent(s) in the
package can pay no fees but be paid for by the child.

*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a parent not
meeting minimum fees on its own. This would allow contracting applications to adjust their fees at
broadcast time instead of overshooting or risking becoming stuck or pinned.

*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as
we do not want a transaction's fees to be double-counted.
To meet the dynamic mempool minimum feerate, i.e., the feerate determined by the transactions
evicted when the mempool reaches capacity (not the static minimum relay feerate), the total package
feerate instead of individual feerate can be used. For example, if the mempool minimum feerate is
5sat/vB and a 1sat/vB parent transaction has a high-feerate child, it may be accepted if
submitted as a package.

*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a presigned
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

transaction (i.e. in which a replacement transaction with a higher fee cannot be signed) being
rejected from the mempool when transaction volume is high and the mempool minimum feerate rises.

Note: Package feerate cannot be used to meet the minimum relay feerate (`-minrelaytxfee`)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

requirement. For example, if the mempool minimum feerate is 5sat/vB and the minimum relay feerate is
set to 5satvB, a 1sat/vB parent transaction with a high-feerate child will not be accepted, even if
submitted as a package.

*Rationale*: Avoid situations in which the mempool contains non-bumped transactions below min relay
feerate (which we consider to have pay 0 fees and thus receiving free relay). While package
submission would ensure these transactions are bumped at the time of entry, it is not guaranteed
that the transaction will always be bumped. For example, a later transaction could replace the
fee-bumping child without still bumping the parent. These no-longer-bumped transactions should be
removed during a replacement, but we do not have a DoS-resistant way of removing them or enforcing a
limit on their quantity. Instead, prevent their entry into the mempool.

Implementation Note: Transactions within a package are always validated individually first, and
package validation is used for the transactions that failed. Since package feerate is only
calculated using transactions that are not in the mempool, this implementation detail affects the
outcome of package validation.

*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as
we do not want a transaction's fees to be double-counted.

*Rationale*: Packages are intended for incentive-compatible fee-bumping: transaction B is a
"legitimate" fee-bump for transaction A only if B is a descendant of A and has a *higher* feerate
than A. We want to prevent "parents pay for children" behavior; fees of parents should not help
Expand Down
2 changes: 2 additions & 0 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ BITCOIN_CORE_H = \
node/minisketchwrapper.h \
node/psbt.h \
node/transaction.h \
node/txpackagetracker.h \
node/txreconciliation.h \
node/utxo_snapshot.h \
node/validation_cache_args.h \
Expand Down Expand Up @@ -410,6 +411,7 @@ libbitcoin_node_a_SOURCES = \
node/minisketchwrapper.cpp \
node/psbt.cpp \
node/transaction.cpp \
node/txpackagetracker.cpp \
node/txreconciliation.cpp \
node/utxo_snapshot.cpp \
node/validation_cache_args.cpp \
Expand Down
1 change: 1 addition & 0 deletions src/Makefile.test.include
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ BITCOIN_TESTS =\
test/translation_tests.cpp \
test/txindex_tests.cpp \
test/txpackage_tests.cpp \
test/txpackagetracker_tests.cpp \
test/txreconciliation_tests.cpp \
test/txrequest_tests.cpp \
test/txvalidation_tests.cpp \
Expand Down
1 change: 1 addition & 0 deletions src/consensus/validation.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ enum class TxValidationResult {
TX_CONFLICT,
TX_MEMPOOL_POLICY, //!< violated mempool's fee/size/descendant/RBF/etc limits
TX_NO_MEMPOOL, //!< this node does not have a mempool so can't validate the transaction
TX_LOW_FEE, //!< fee was insufficient to meet some policy (minimum/RBF/etc)
};

/** A "reason" why a block was invalid, suitable for determining whether the
Expand Down
5 changes: 4 additions & 1 deletion src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
#include <node/mempool_args.h>
#include <node/mempool_persist_args.h>
#include <node/miner.h>
#include <node/txpackagetracker.h>
#include <node/txreconciliation.h>
#include <node/validation_cache_args.h>
#include <policy/feerate.h>
Expand Down Expand Up @@ -488,6 +489,7 @@ void SetupServerArgs(ArgsManager& argsman)
argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-i2pacceptincoming", strprintf("Whether to accept inbound I2P connections (default: %i). Ignored if -i2psam is not set. Listening for inbound I2P connections is done through the SAM proxy, not by binding to a local address and port.", DEFAULT_I2P_ACCEPT_INCOMING), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only to network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-packagerelay", strprintf("[EXPERIMENTAL] Support relaying transaction packages (default: %u)", node::DEFAULT_ENABLE_PACKAGE_RELAY), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
Expand Down Expand Up @@ -1551,10 +1553,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
}

ChainstateManager& chainman = *Assert(node.chainman);
const bool enable_package_relay{gArgs.GetBoolArg("-packagerelay", node::DEFAULT_ENABLE_PACKAGE_RELAY)};

assert(!node.peerman);
node.peerman = PeerManager::make(*node.connman, *node.addrman, node.banman.get(),
chainman, *node.mempool, ignores_incoming_txs);
chainman, *node.mempool, ignores_incoming_txs, enable_package_relay);
RegisterValidationInterface(node.peerman.get());

// ********************************************************* Step 8: start indexers
Expand Down
16 changes: 14 additions & 2 deletions src/kernel/mempool_persist.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
}
uint64_t num;
file >> num;
LOCK2(cs_main, pool.cs);
while (num) {
--num;
CTransactionRef tx;
Expand All @@ -77,8 +78,12 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
pool.PrioritiseTransaction(tx->GetHash(), amountdelta);
}
if (nTime > TicksSinceEpoch<std::chrono::seconds>(now - pool.m_expiry)) {
LOCK(cs_main);
const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/false, /*test_accept=*/false);
// Use bypass_limits=true to skip feerate checks, and call TrimToSize() at the very
// end. This means the mempool may temporarily exceed its maximum capacity. However,
// this means fee-bumped transactions are persisted, and the resulting mempool
// minimum feerate is not dependent on the order in which transactions are loaded
// from disk.
const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/true, /*test_accept=*/false);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (accepted.m_result_type == MempoolAcceptResult::ResultType::VALID) {
++count;
} else {
Expand All @@ -104,6 +109,13 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
for (const auto& i : mapDeltas) {
pool.PrioritiseTransaction(i.first, i.second);
}
const auto size_before_trim{pool.size()};
// Ensure the maximum memory limits are ultimately enforced and any transactions below
// minimum feerates are evicted, since bypass_limits was set to true during ATMP calls.
pool.TrimToSize(pool.m_max_size_bytes);
const auto num_evicted{size_before_trim - pool.size()};
count -= num_evicted;
failed += num_evicted;

std::set<uint256> unbroadcast_txids;
file >> unbroadcast_txids;
Expand Down
3 changes: 3 additions & 0 deletions src/logging.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ const CLogCategoryDesc LogCategories[] =
{BCLog::BLOCKSTORE, "blockstorage"},
{BCLog::TXRECONCILIATION, "txreconciliation"},
{BCLog::SCAN, "scan"},
{BCLog::TXPACKAGES, "txpackages"},
{BCLog::ALL, "1"},
{BCLog::ALL, "all"},
};
Expand Down Expand Up @@ -286,6 +287,8 @@ std::string LogCategoryToStr(BCLog::LogFlags category)
return "txreconciliation";
case BCLog::LogFlags::SCAN:
return "scan";
case BCLog::LogFlags::TXPACKAGES:
return "txpackages";
case BCLog::LogFlags::ALL:
return "all";
}
Expand Down
1 change: 1 addition & 0 deletions src/logging.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ namespace BCLog {
BLOCKSTORE = (1 << 26),
TXRECONCILIATION = (1 << 27),
SCAN = (1 << 28),
TXPACKAGES = (1 << 29),
ALL = ~(uint32_t)0,
};
enum class Level {
Expand Down
Loading