Add package acceptance logic to mempool

sdaftuar commented at 8:16 pm on July 16, 2019: member

Accepting a single transaction to the mempool only succeeds if (among other things) the feerate of the transaction is greater than both the min relay fee and the mempool min fee. Consequently, a transaction below the minimum fee may not be accepted to the mempool, even if we later learn of a transaction with a high fee that depends on it.

This PR adds code to validation that will accept a package of transactions to the mempool under the following conditions:

All package transactions must be direct parents of the final transaction. This is a simple heuristic for ensuring that a candidate list of transactions is in fact a package (we wouldn’t want arbitrary transactions to be paying for random low feerate transactions)
The feerate of the package, as a whole, exceeds the mempool min fee and the min relay fee.
No transactions in the mempool conflict with any transactions in the package. This is a simplification that makes the logic easier to write. Without this requirement, we would need to do additional checks to ensure that no parent transaction would evict a transaction from the mempool that some other child depends on.
The ancestor/descendant size limits are calculated assuming that any mempool ancestor of any candidate transaction is an ancestor of all the candidate transactions. This allows for doing simpler calculations to ensure that we’re staying within the mempool’s package limits. If we eliminated this, we would need to do much more careful package calculations for each candidate transaction and each in-mempool ancestor.

This PR also adds code to net_processing that will attempt to process transaction packages in one narrow case: if a transaction fails to get into the mempool due to insufficient fee, but has an orphan in the orphan pool, then we will try to process the pair together to see if they can be accepted as a package.

This PR is definitely WIP, but I’m opening it as a proof-of-concept motivation for refactoring ATMP (#16400).

DrahtBot commented at 10:33 pm on July 16, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17985 (net: Remove forcerelay of rejected txs by MarcoFalke)
#17399 (validation: Templatize ValidationState instead of subclassing by jkczyz)
#15606 ([experimental] UTXO snapshots by jamesob)

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.

DrahtBot added the label Mempool on Jul 17, 2019

DrahtBot added the label P2P on Jul 17, 2019

DrahtBot added the label Tests on Jul 17, 2019

DrahtBot added the label Validation on Jul 17, 2019

sdaftuar force-pushed on Jul 24, 2019

in src/validation.h:334 in 149930d746 outdated

329@@ -326,7 +330,8 @@ bool TestLockPointValidity(const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_mai
330  *
331  * See consensus/consensus.h for flag definitions.
332  */
333-bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
334+bool CheckSequenceLocks(const CTxMemPool &pool, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
335+bool CheckSequenceLocks(CCoinsViewCache& view, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

ajtowns commented at 1:24 am on July 25, 2019:

Moving the & in CTxMemPool &pool seems like a typo. Also you don’t actually need the view method in the header, could just make it a static.

sdaftuar commented at 7:23 pm on July 29, 2019:

Fixed.

in src/net_processing.cpp:2567 in 149930d746 outdated

2571+            // in the orphan pool -- then maybe that tx is only missing this
2572+            // one parent.
2573+            // Try to process the pair as a package.
2574+            bool added_as_package = false;
2575+            if (state.GetRejectCode() == REJECT_INSUFFICIENTFEE) {
2576+                LOCK(g_cs_orphans);

ajtowns commented at 1:27 am on July 25, 2019:

g_cs_orphans is already locked earlier in the path (LOCK2(cs_main, g_cs_orphans))

sdaftuar commented at 7:23 pm on July 29, 2019:

Fixed.

in src/validation.cpp:467 in 149930d746 outdated

476+
477+    // Single transaction acceptance
478+    bool AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
479+
480+    // Multiple transaction acceptance
481+    bool AcceptMultipleTransactions(std::list<CTransactionRef> tx_list, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

ajtowns commented at 1:36 am on July 25, 2019:

tx_list should be const & afaics.

sdaftuar commented at 7:23 pm on July 29, 2019:

Fixed.

in src/validation.cpp:1011 in 149930d746 outdated

1271+    AssertLockHeld(cs_main);
1272+    LOCK(m_pool.cs);
1273+
1274+    // We will disable the individual transaction fee-rate checks for all
1275+    // parents of the final transaction.
1276+    ATMPArgs args_bypass_limits { args.m_chainparams, args.m_state, args.m_missing_inputs, args.m_accept_time, args.m_replaced_transactions, /*m_bypass_limits*/ true, args.m_absurd_fee, args.m_coins_to_uncache, args.m_test_accept };

ajtowns commented at 1:54 am on July 25, 2019:

Adding a Workspace::m_cpfpable bool set to true for all but the last tx, and setting const bool bypass_limits = args.bypass_limits || ws.cpfpable seems better to me, fwiw.

sdaftuar commented at 7:24 pm on July 29, 2019:

Looks good, done.

in src/validation.cpp:1015 in 149930d746 outdated

1275+    // parents of the final transaction.
1276+    ATMPArgs args_bypass_limits { args.m_chainparams, args.m_state, args.m_missing_inputs, args.m_accept_time, args.m_replaced_transactions, /*m_bypass_limits*/ true, args.m_absurd_fee, args.m_coins_to_uncache, args.m_test_accept };
1277+    std::list<Workspace> tx_workspaces;
1278+
1279+    for (const CTransactionRef& ptx : tx_list) {
1280+        tx_workspaces.emplace_back(Workspace(ptx));

ajtowns commented at 1:55 am on July 25, 2019:

adding Workspace& ws = tx_workspaces.back(); maybe makes this loop a bit tidier?

sdaftuar commented at 7:24 pm on July 29, 2019:

Agreed.

in src/validation.cpp:1132 in 149930d746 outdated

1366+        return args.m_state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-package-mempool-chain", strprintf("exceeds ancestor size limit for tx %s [limit: %u]", tx_list.back()->GetHash().ToString(), m_limit_ancestor_size));
1367+    }
1368 
1369+    // Make sure all transactions are ancestors of the last one.
1370+    // For now, just check that the last transaction has all prior transactions
1371+    // as direct inputs. We can relax this in the future for bigger packages.

ajtowns commented at 2:12 am on July 25, 2019:

This check could be done first, before even setting up the workspaces, as far as I can see.

in src/validation.cpp:1168 in 149930d746 outdated

1404+        // success. We have to wait until all the inputs are in the mempool or
1405+        // in the utxo set (for now) before we can invoke this. This should
1406+        // not fail unless there's a logic bug in our script validation, but if
1407+        // it somehow were to fail on some child tx, we would potentially be
1408+        // allowing parents into the mempool with this logic.
1409+        if (!ConsensusScriptChecks(args, *wit, txdata[i])) return false;

ajtowns commented at 2:17 am on July 25, 2019:

This seems like it would be a good case for the CHECK macro from #16136 (if it logged failures, anyway)…

in src/validation.cpp:1161 in 149930d746 outdated

1397+    if (args.m_test_accept) return true;
1398+
1399+    // Add everything to the mempool, and make sure the last transaction makes
1400+    // it in.
1401+    size_t i=0;
1402+    for (auto wit = tx_workspaces.begin(); wit != tx_workspaces.end(); ++wit, ++i) {

ajtowns commented at 2:18 am on July 25, 2019:

Would be nice if C++11 had a “zip” iterator… FWIW, you could do this as:

0struct WorkspaceAndTxData { Workspace ws; PrecomputedTransactionData* ptxdata; };
1std::list<WorkspaceAndTxData> tx_workspaces;
2...
3for (auto& wstxd : tx_workspaces) {
4    txdata.emplace_back(*wstxd.ws.m_ptx);
5    wstxd.ptxdata = &txdata.back();
6}

and only have to loop over tx_workspaces, but it’s probably not worth the hassle.

in src/validation.cpp:1123 in 149930d746 outdated

1357     }
1358 
1359-    GetMainSignals().TransactionAddedToMempool(ptx);
1360+    // In case we have no in-mempool ancestors, we must check the transaction
1361+    // package itself.
1362+    if (total_size > m_limit_descendant_size) {

ajtowns commented at 2:30 am on July 25, 2019:

Might as well do this test before looping over all_ancestors

in src/validation.cpp:1004 in 149930d746 outdated

1264+
1265+    return true;
1266+}
1267+
1268+
1269+bool MemPoolAccept::AcceptMultipleTransactions(std::list<CTransactionRef> tx_list, ATMPArgs& args)

ajtowns commented at 3:22 am on July 25, 2019:

I think once this is more baked, it’d be good to pull sections of this out into their own private MemPoolAccept:: methods so the overall logic in this function is as clear as the logic in AcceptSingleTransaction

ajtowns commented at 3:33 am on July 25, 2019: member

I think this works as a proof of concept.

sdaftuar force-pushed on Jul 31, 2019

sdaftuar force-pushed on Sep 19, 2019

in src/txmempool.h:756 in cd8f11aaaa outdated

750@@ -751,8 +751,12 @@ class CTxMemPool
751  */
752 class CCoinsViewMemPool : public CCoinsViewBacked
753 {
754+public:
755+    void AddPotentialTransaction(const CTransactionRef& ptx) { package_tx.emplace(ptx->GetHash(), ptx); }

NicolasDorier commented at 8:21 am on September 30, 2019:

As far as I see, in the current code code paths, the max number of elements in package_tx is two. But maybe there should be a hard limit, even if high, at this level just in case?

in src/validation.cpp:1178 in 92cc72905a outdated

1186+        if (wit != tx_workspaces.begin()) {
1187+            wit->m_ancestors.clear();
1188+            std::string dummy_string;
1189+            // Don't worry about the return value here; it must pass based on
1190+            // the checks above. TODO: assert on passing?
1191+            m_pool.CalculateMemPoolAncestors(*(wit->m_entry), wit->m_ancestors, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants, m_limit_descendant_size, dummy_string);

etscrivner commented at 2:57 pm on October 1, 2019:

This is probably also a good spot for a DCHECK just to ensure that the assumption - “this must pass based on the checks above” - holds here.

in test/functional/feature_package_relay.py:72 in cd8f11aaaa outdated

67+        self.nodes[0].add_p2p_connection(BaseNode())
68+
69+        self.nodes[1].generate(101)
70+        self.sync_all(self.nodes[0:2])
71+
72+        # On node1, generate a 0-fee transaction, and then a 2-satoshi-per-byte

pinheadmz commented at 1:37 pm on October 2, 2019:

I might be misunderstanding, but it looks like the first tx fee rate is set to 2 sat/kB (actual total fee on this tx comes out to a single sat) and the second tx fee rate is set to 50000 (actual total fee ~0.000083)?

etscrivner commented at 2:34 pm on October 2, 2019: contributor

Agree with others that I think this works as a proof-of-concept. In my opinion, this still needs more work on tests, more extensive testing, and generalization beyond 2 transaction packages (up to some maximum package size).

in src/net_processing.cpp:2587 in cd8f11aaaa outdated

2591+            // If this tx didn't make it in due to feerate, and there is a tx
2592+            // in the orphan pool -- then maybe that tx is only missing this
2593+            // one parent.
2594+            // Try to process the pair as a package.
2595+            bool added_as_package = false;
2596+            if (state.GetRejectCode() == REJECT_INSUFFICIENTFEE) {

fjahr commented at 4:22 pm on October 2, 2019:

Maybe also add a check for g_orphan_list.size() > 0 here? But I am not experienced with the orphan pool so it may be fair to leave it out if the orphan pool is populated 99% of the time.

fjahr commented at 4:33 pm on October 2, 2019: member

This works for me as a proof of concept. Obviously the tests still need some work but thanks for the extensive commenting in the code.

jonatack commented at 4:42 pm on October 2, 2019: member

Concept/approach ACK cd8f11aaaa7dd27f95af3d4cb85ff7c7607e76f9, tests pass here as well as rebased on master, node running since a few hours with net logging. The code documentation is helpful. Am still reviewing the code, logic, and test, particularly the checks to satisfy and the complexity introduced in AcceptMultipleTransactions().

Suggestion to reviewers: It may be helpful to review merged PR #16400 and [issue #14895](https://github.com/bitcoin/bitcoin/issues/14895) before this one.

Given that this PR is labelled Draft and the ATMP refactoring in #16400 has been merged, what are your plans going forward beyond concept/approach ACKs? For example, continue refactoring the p2p protocol for package relay computational efficiency/simplicity/multiparents, or moving forward with a standalone PoC implementation like this one?

in test/functional/feature_package_relay.py:102 in cd8f11aaaa outdated

 97+
 98+        self.nodes[0].generate(1) # Clear out the reject filter
 99+
100+        # Delivering tx2 should result it being in the orphan pool, and then
101+        # delivering tx should cause both to be accepted.
102+        p2p = self.nodes[0].add_p2p_connection(BaseNode())

pinheadmz commented at 5:26 pm on October 2, 2019:

Couldn’t this p2p variable be defined up on line 67?

etscrivner commented at 5:55 pm on October 2, 2019: contributor

Add some additional tests in this gist and have also addressed the TODO item: https://gist.github.com/etscrivner/19d5f942a973940aaaeb397bc5e0e0d9

in src/validation.cpp:307 in 92cc72905a outdated

307@@ -311,6 +308,14 @@ bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flag
308     return EvaluateSequenceLocks(index, lockPair);
309 }
310 
311+bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp, bool useExistingLockPoints)

ariard commented at 10:53 pm on October 31, 2019:

I think the new wrapper doesn’t do what the removed comment intended? Can’t we pass coins_cache defined above instead of defining a new one, would work too in removeForReorg ? Also isn’t this a behavior change as coin is fetched from UTXO set instead from mempool?

This change is may worth its own commit.

in src/validation.cpp:238 in 92cc72905a outdated

238@@ -239,10 +239,9 @@ bool TestLockPointValidity(const LockPoints* lp)
239     return true;
240 }
241 
242-bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp, bool useExistingLockPoints)
243+static bool CheckSequenceLocks(CCoinsViewCache &view, const CTransaction& tx, int flags, LockPoints* lp, bool useExistingLockPoints=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

ariard commented at 10:58 pm on October 31, 2019:

EXCLUSIVE_LOCKS_REQUIRED(pool.cs, cs_main)? (wrapper and AcceptMultipleTransactions too)

in src/validation.cpp:711 in 92cc72905a outdated

717@@ -709,8 +718,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
718         return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "bad-txns-too-many-sigops",
719                 strprintf("%d", nSigOpsCost));
720 
721-    // No transactions are allowed below minRelayTxFee except from disconnected
722-    // blocks
723+    // No transactions are allowed below minRelayTxFee/mempool min fee except
724+    // from disconnected blocks

ariard commented at 11:03 pm on October 31, 2019:

“or being CPFP’ed by another tx”

in src/txmempool.h:760 in 92cc72905a outdated

750@@ -751,8 +751,12 @@ class CTxMemPool
751  */
752 class CCoinsViewMemPool : public CCoinsViewBacked
753 {
754+public:
755+    void AddPotentialTransaction(const CTransactionRef& ptx) { package_tx.emplace(ptx->GetHash(), ptx); }
756+
757 protected:
758     const CTxMemPool& mempool;
759+    std::map<uint256, const CTransactionRef> package_tx;

ariard commented at 11:23 pm on October 31, 2019:

Could have a better name like package_pool just to underscore we have multiple pending txn. I didn’t get the rational at first read, I think you can explain given we have mempool locked and can’t write until we have the whole set of tx, we need this temporary buffer to find outpoints of package txn after first row of parents. If this to work assuming topological ordering it should be documented?

in src/validation.cpp:1063 in 92cc72905a outdated

1071+        Workspace &ws = tx_workspaces.back();
1072+
1073+        if (!PreChecks(args, ws)) return false;
1074+
1075+        // For now, do not allow replacements in package transactions. If we
1076+        // relax this, we would need to check that no child transaction depends

ariard commented at 11:48 pm on October 31, 2019:

+1 for conflicts isolation for now. But IMO we may have to solve child conflicts in a way or another if people start to use one CPFP to bump multiple multi-party txn, like Alice broadcast commitment tx 1 + CPFP child tx, txn get into mempool, now she have to broadcast commitment tx 2 but doesn’t have another UTXO available and can’t CPFP output as going beyond carve-out relaxation..

in src/validation.cpp:1155 in 92cc72905a outdated

1163+    }
1164+
1165+    // This package should be accepted except possibly for failing in
1166+    // TrimToSize(), which we can't exercise without actually adding to the
1167+    // mempool and seeing what would happen. Note that we are not adding
1168+    // these transactions to the script cache, unlike in the single-tx case.

ariard commented at 0:35 am on November 1, 2019:

Rational?

ariard commented at 0:53 am on November 1, 2019: member

Reviewed only mempool/validation change for now.

No transactions in the mempool conflict with any transactions in the package. This is a simplification that makes the logic easier to write. Without this requirement, we would need to do additional checks to ensure that no parent transaction would evict a transaction from the mempool that some other child depends on.

I assume we can still RBF both parents or CPFP child in case our package txn are still stucking in the mempool? It shouldn’t be an issue as after acceptance txn are normal mempool txn, assuming we don’t have package garbage value for ancestors/descendants.

The ancestor/descendant size limits are calculated assuming that any mempool ancestor of any candidate transaction is an ancestor of all the candidate transactions.

It should be fine for LN as unconfirmed ancestors should be limited to 2 (commitment tx and CPFP) unless people batch their commitment txn with one CPFP.

in src/net_processing.cpp:746 in 1dd7de23f2 outdated

747@@ -748,6 +748,20 @@ void RequestTx(CNodeState* state, const uint256& txid, std::chrono::microseconds
748     peer_download_state.m_tx_process_time.emplace(process_time, txid);
749 }
750 
751+// Add to a peer's orphan_work_set after processing a given transaction.
752+void UpdateOrphanWorkSet(const CTransaction& tx, CNode *peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)

ariard commented at 4:50 pm on November 1, 2019:

Can be reused too in ProcessOrphanTx

in src/net_processing.cpp:2582 in 1dd7de23f2 outdated

2600+                    if (it != mapOrphanTransactionsByPrev.end()) {
2601+                        for (auto orphan_iter : it->second) orphans_missing_this_tx.push_back(orphan_iter);
2602+                    }
2603+                }
2604+                if (!orphans_missing_this_tx.empty()) {
2605+                    const COrphanTx &orphan_tx = orphans_missing_this_tx.front()->second;

ariard commented at 5:33 pm on November 1, 2019:

If I broadcast a second CPFP due to to the first one being still too low but this one still being in the orphan pool, you need to iter and try with the whole set not only picking up the first transaction ? That would be a DoS vector bounded by the size of the orphan pool, and maybe it can be mitigate by caching the package already-tried.

in src/net_processing.cpp:2608 in 1dd7de23f2 outdated

2626+                                    mempool.size(), mempool.DynamicMemoryUsage() / 1000);
2627+                        }
2628+
2629+                        // Recursively process any orphan transactions that depended on these
2630+                        ProcessOrphanTx(connman, pfrom->orphan_work_set, lRemovedTxn);
2631+                    }

ariard commented at 6:09 pm on November 1, 2019:

If package wasn’t accepted (like a high-fee invalid orphan tx), and if inputs are not missing, if orphan_tx is invalid we may punish peer, or at least erase it for not being standard or insufficient fee?

ariard commented at 6:23 pm on November 1, 2019: member

It should be made really clear for off-chain protocol devs to always rebroadcast the whole package instead of only the parent or the child. I’m worrying about scenarii where a a commitment tx is broadcast, don’t get into the mempool neither orphan one, latter the high-fee child is broadcast at the application logic and get into the orphan, an attacker overfulfill the orphan pool to discard the child, the application logic rebroadcast the parent tx but not the CPFP assuming it’s already there…

That’s said, I think the P2P work on principle but could be made more robust and tests should include multiple bumped childs and orphan pool overflow.

sdaftuar marked this as ready for review on Jan 8, 2020

Add package-acceptance logic to mempool

Accepting a single transaction to the mempool only succeeds if (among other
things) the feerate of the transaction is greater than both the min relay fee
and the mempool min fee. Consequently, a transaction below the minimum fee
may not be accepted to the mempool, even if we later learn of a transaction
with a high relay fee that depends on it.

This commit adds a function that will accept a package of transactions to the
mempool, with the following restrictions:

- All package transactions must be direct parents of the final transaction.
  This is a simple heuristic for ensuring that a candidate list of transactions
  is in fact a package (we wouldn't want arbitrary transactions to be paying
  for random low feerate transactions)

- The feerate of the package, as a whole, exceeds the mempool min fee and the
  min relay fee.

- No transactions in the mempool conflict with any transactions in the package.
  This is a simplification that makes the logic easier to write. Without this
  requirement, we would need to do additional checks to ensure that no parent
  transaction would evict a transaction from the mempool that some other child
  depends on.

- The ancestor/descendant size limits are calculated assuming that any mempool
  ancestor of any candidate transaction is an ancestor of all the candidate
  transactions.
  This allows for doing simpler calculations to ensure that we're staying
  within the mempool's package limits. If we eliminated this, we would need to
  do much more careful package calculations for each candidate transaction and each
  in-mempool ancestor.

This commit does not include any accessor function for utilizing this logic (eg
by exposing this function at the p2p or rpc layer).

c067d85500

DrahtBot added the label Needs rebase on Jan 8, 2020

ariard commented at 1:35 am on January 9, 2020: member

In fact worry described here holds, I’ve modified the new test a bit to overflow the orphan pool with dumb-but-valid orphan pools and it succeeds to evict high-fee child tx easily, even with a low number of malicious orphans (200) : https://github.com/ariard/bitcoin/commit/e20ad2a44a830df7f04cf9bccf9e6791b73d2527

Implement package relay for 2-tx packages from the orphan pool

Exposes a simple use case for package relay -- if we receive a child
transaction that is missing a parent, then we request the parent from our
peers.

If a peer responds with a transaction that is rejected from the mempool due to
feerate, we have an opportunity to accept that parent along with the child, if
the child's feerate is sufficiently high and the child is missing no other
parents.

6a3bdba074

sdaftuar force-pushed on Jan 9, 2020

sdaftuar commented at 4:01 am on January 9, 2020: member

It should be made really clear for off-chain protocol devs to always rebroadcast the whole package instead of only the parent or the child. I’m worrying about scenarii where a a commitment tx is broadcast, don’t get into the mempool neither orphan one, latter the high-fee child is broadcast at the application logic and get into the orphan, an attacker overfulfill the orphan pool to discard the child, the application logic rebroadcast the parent tx but not the CPFP assuming it’s already there… @ariard It wasn’t my goal in this PR to deploy a new p2p package relay scheme that is resilient to DoS attack; instead I wanted to take a smaller use case (resolving orphan transactions missing a single low fee parent) to motivate adding the package acceptance logic.

I think once we have the mempool package acceptance logic in, we could then improve the p2p layer further to allow fancier relay schemes. (See also my comment in #14895 (comment).)

I’ll update the title of the PR to make this more clear.

sdaftuar renamed this:
~~Package relay~~
Add package acceptance logic to mempool
on Jan 9, 2020

DrahtBot removed the label Needs rebase on Jan 9, 2020

ariard commented at 5:10 pm on January 9, 2020: member

Thanks for clarifying scope of this PR. I do think too it’s good to split this project in multi-parts to focus on new mempool acceptance logic, specially what the worst width/depth of a chain of messy transactions we select as a package submission DoS bound.

That’s said, we may reduce the risks of mempool CPU DoS by implementing the right measures at the p2p level like rate-limiting per-peer package reception. Also if we have different p2p package relay mechanisms (receiver-initiated, sender p2p messages, …) we need to think how we coordinate them between different releases to avoid applications relying on the wrong ones for their uses cases and falsely assuming they are secure.

JeremyRubin added this to the "Package Relay" column in a project

sdaftuar commented at 2:55 pm on January 29, 2020: member

I’m starting to think that perhaps the use case here isn’t really worth it – I tried to split off what I thought would be a simple use case (of processing some orphan transactions as packages with their rejected parents), but it turns out to be a bit trickier than I expected, and given that we’d just throw this away if we implemented an actual package relay protocol anyway this seems like wasted effort.

sdaftuar closed this on Jan 29, 2020

DrahtBot locked this on Feb 15, 2022

Add package acceptance logic to mempool #16401

Conflicts