Conservatively accept RBF bumps bumping one tx at the package limits #16421

pull TheBlueMatt wants to merge 1 commits into bitcoin:master from TheBlueMatt:2019-07-lightning-policy-bump changing 2 files +51 −3
  1. TheBlueMatt commented at 9:19 pm on July 18, 2019: member

    Based on #15681, this adds support for some simple cases of RBF inside of large packages. Issue pointed out by sdaftuar in #15681, and this fix (or a broader one) is required to make #15681 fully useful.

    Accept RBF bumps of single transactions (ie which evict exactly one transaction) even when that transaction is a member of a package which is currently at the package limit if the new transaction does not add any additional mempool dependencies from the original.

    This could be made a bit looser in the future and still be safe, but for now this fixes the case that a transaction which was accepted by the carve-out rule will not be directly RBF’able

  2. DrahtBot added the label Tests on Jul 18, 2019
  3. DrahtBot added the label Validation on Jul 18, 2019
  4. DrahtBot commented at 11:28 pm on July 18, 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:

    • #16400 (refactor: Rewrite AcceptToMemoryPoolWorker() using smaller parts by sdaftuar)

    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.

  5. TheBlueMatt force-pushed on Jul 18, 2019
  6. fanquake requested review from sdaftuar on Jul 21, 2019
  7. jnewbery commented at 9:34 pm on July 22, 2019: member
    Please rebase so this only contains the single commit for this PR.
  8. TheBlueMatt commented at 8:08 pm on July 23, 2019: member
    Rebased. Now just one commit + master.
  9. TheBlueMatt force-pushed on Jul 23, 2019
  10. laanwj added this to the "Blockers" column in a project

  11. TheBlueMatt force-pushed on Jul 29, 2019
  12. TheBlueMatt commented at 6:36 pm on July 29, 2019: member
    Rebased after #16471. Also, @sdaftuar noted that I (a) misunderstood the meaning of setConflicts so the description in the comment was wrong in a few ways, and also we were allowing an RBF transaction to get in via the carve-out even when it shouldn’t be allowed to (ie when it has more than one mempool ancestor). Sadly, after talking through it it doesn’t seem like we’ll be able to get away with no extra calls to CalculateMemPoolAncestors, but at least its a bit easier to reason about now (IMO).
  13. in src/validation.cpp:655 in e8bd0c5ee1 outdated
    652+                std::set<uint256> conflict_existing_mempool_inputs;
    653+                for (const CTxIn& input : conflict->GetTx().vin) {
    654+                    if (mempool.exists(input.prevout.hash)) {
    655+                        conflict_existing_mempool_inputs.insert(input.prevout.hash);
    656+                    }
    657+                }
    


    sdaftuar commented at 6:53 pm on July 29, 2019:
    Can we just use GetMemPoolParents(conflict) and grab the hashes from there, instead of iterating all the inputs?

    TheBlueMatt commented at 5:57 pm on July 30, 2019:
    No, GetMemPoolParents just returns the parents set, which isn’t actually filled until the very last step in addUnchecked.

    sdaftuar commented at 6:11 pm on July 30, 2019:
    The conflict tx is already in the mempool, so shouldn’t that work fine?

    TheBlueMatt commented at 9:05 pm on July 31, 2019:
    Oh right, was looking at the wrong line and thought you meant the new entry. Will fix.
  14. in src/validation.cpp:663 in e8bd0c5ee1 outdated
    660+                    if (mempool.exists(new_input.prevout.hash)) {
    661+                        if (conflict_existing_mempool_inputs.count(new_input.prevout.hash) == 0) {
    662+                            return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", dummy_err_string);
    663+                        }
    664+                    }
    665+                }
    


    sdaftuar commented at 6:56 pm on July 29, 2019:
    Similarly, if you hadn’t cleared out setAncestors() a few lines up, I think you could skip having to walk all the inputs again here. Perhaps save that and then compare setAncestors to the mempool parents of conflictTx?

    TheBlueMatt commented at 6:00 pm on July 30, 2019:
    Right, but doesn’t CalculateMemPoolAncestors return early if we hit a limit? I didn’t want to start introducing some “if we return early, X is initialized, but Y is not” kind of invariants in CalculateMemPoolAncestors. If you think its worth it, though…

    sdaftuar commented at 6:06 pm on July 30, 2019:
    Oh right, never mind.
  15. in src/validation.cpp:664 in e8bd0c5ee1 outdated
    661+                        if (conflict_existing_mempool_inputs.count(new_input.prevout.hash) == 0) {
    662+                            return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", dummy_err_string);
    663+                        }
    664+                    }
    665+                }
    666+                if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants + 1,
    


    sdaftuar commented at 6:59 pm on July 29, 2019:

    I think a comment would be helpful here to explain exactly what we’re doing:

    • No need to bump ancestor sizes/limits (since those are only tested on the transaction itself and are unaffected by the presence of conflicting transactions)
    • Descendant size / count are bumped by conflict tx size and 1, to account for the removal of that transaction from all the ancestors of the new transaction. Since we require that the new transaction not introduce any new mempool-ancestors (compared with the conflict tx), this is sufficient to enforce our existing package limits.
  16. in src/validation.cpp:667 in e8bd0c5ee1 outdated
    663+                        }
    664+                    }
    665+                }
    666+                if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants + 1,
    667+                        nLimitDescendantSize + conflict->GetTxSize(), dummy_err_string)) {
    668+                    if (nSize >  EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
    


    sdaftuar commented at 7:01 pm on July 29, 2019:
    Another comment would be helpful here, to explain that if we failed to RBF due to the package limit, we’ll give it one more try using the carve-out rules. So we use the same ancestor limits that the carve-out provision requires, but bump the descendant limits to account for the to-be-removed transaction.
  17. in src/validation.cpp:668 in e8bd0c5ee1 outdated
    664+                    }
    665+                }
    666+                if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants + 1,
    667+                        nLimitDescendantSize + conflict->GetTxSize(), dummy_err_string)) {
    668+                    if (nSize >  EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
    669+                            !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 2,
    


    sdaftuar commented at 7:02 pm on July 29, 2019:
    It looks like you’re not clearing setAncestors before this call. I think that’s actually fine, but maybe better practice to clear it out to avoid confusion?
  18. in src/validation.cpp:624 in e8bd0c5ee1 outdated
    632-            // outputs - one for each counterparty. For more info on the uses for
    633-            // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
    634-            if (nSize >  EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
    635-                    !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) {
    636-                return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
    637+            if (setConflicts.size() == 1) {
    


    sdaftuar commented at 7:03 pm on July 29, 2019:
    This whole section could probably use a clearer comment that lays out the problem we have with evaluating package limits in the presence of RBF transactions, to help future readers of this code.

    instagibbs commented at 2:18 pm on August 19, 2019:
    for readers of the comments, this has (attempted) to be addressed
  19. sdaftuar commented at 7:04 pm on July 29, 2019: member
    Looks on the right track to me, though I still need to think about the logic from scratch again and do some testing.
  20. TheBlueMatt commented at 6:46 pm on July 30, 2019: member
    Rewrote the comment at the top of the new block to now be a Mega Comment (tm). Hopefully its sufficient, though I can break it up and move it to corresponding code if you really want @sdaftuar, I just didn’t bother cause its easy to rewrite without.
  21. TheBlueMatt force-pushed on Jul 30, 2019
  22. MarcoFalke commented at 9:06 pm on July 30, 2019: member
    Concept ACK, though I’d prefer if this was solved for all mempool txs and not only for a special case of lightning txs.
  23. TheBlueMatt force-pushed on Jul 31, 2019
  24. TheBlueMatt commented at 9:07 pm on July 31, 2019: member
    @MarcoFalke This isn’t completely targeted only at lightning/carve-out/contract applications, though it obviously ensures that carve-out transactions are a supported use-case. Mostly trying to implement this in the general case is Hard (tm), but this does work for any only-directly-conflicts-with-1-transaction tx.
  25. MarcoFalke removed the label Tests on Aug 1, 2019
  26. ajtowns commented at 3:46 am on August 15, 2019: member

    ACK ab603961f13f09ff18877e732ed87705ed23db52 ; code review, check test failed before patch and succeeds afterwards.

    I think the “RBFs a single tx without adding additional unconfirmed inputs” makes sense as a reasonably general rule, and doesn’t seem especially lightning specific.

  27. in src/validation.cpp:657 in ab603961f1 outdated
    620@@ -620,19 +621,70 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
    621             setAncestors.clear();
    


    ajtowns commented at 3:53 am on August 15, 2019:

    Rather than doing this test after the initial CalculateMemPoolAncestors(), would it make sense to work out if the limits can be relaxed first, and then start calling CalculateMemPoolAncestors? ie

     0int bump_desc = 0;
     1int bump_desc_size = 0;
     2if (setConflicts.size() == 1) {
     3 auto conflict = *setIterConflicting.begin();
     4 if (tx_only_uses_unconfirmed_inputs_from_conflicting_tx) {
     5  bump_desc = 1;
     6  bump_desc_size = conflict->GetTxSize();
     7 }
     8}
     9if (!CalculateMemPoolAncestors(..,nLimitDescendents+bump_desc, nLimitDescendantSize+bump_desc_size,..) {
    10 std::string dummy_err;
    11 if (nSize >  EXTRA_DESCENDANT_TX_SIZE_LIMIT || !CalculateMemPoolAncestors(..+bump_desc,..+bump_desc_size)) {
    12  Invalid();
    13 }
    14}
    

    TheBlueMatt commented at 9:52 pm on August 19, 2019:
    Ah, indeed, nice call, done.
  28. in src/validation.cpp:625 in ab603961f1 outdated
    633-            // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
    634-            if (nSize >  EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
    635-                    !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) {
    636-                return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
    637+            if (setConflicts.size() == 1) {
    638+                // In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we
    


    ajtowns commented at 3:55 am on August 15, 2019:
    I wonder if extensive comments like this wouldn’t be better as an informative BIP? Also it’s probably a lost cause, but wrapping the comments at 80 columns or less rather than ~120 would be nice :)

    TheBlueMatt commented at 9:53 pm on August 19, 2019:

    80 chars is….very antiquated. I dunno anyone who is left coding on a 80x60 terminal :p.

    Still, I don’t think this deserves a BIP if only cause we hopefully can extend it further in the future.

  29. in src/validation.cpp:629 in ab603961f1 outdated
    637+            if (setConflicts.size() == 1) {
    638+                // In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we
    639+                // would meet the chain limits after the conflicts have been removed. However, there isn't a practical
    640+                // way to do this short of calculating the ancestor and descendant sets with an overlay cache of
    641+                // changed mempool entries. Due to both implementation and runtime complexity concerns, this isn't
    642+                // very realistic, either, thus we we try to ensure a limited set of transaction is RBF'able despite
    


    instagibbs commented at 2:07 pm on August 19, 2019:
    too many commas :) s/very realistic, either/very realistic either/
  30. in src/validation.cpp:634 in ab603961f1 outdated
    642+                // very realistic, either, thus we we try to ensure a limited set of transaction is RBF'able despite
    643+                // mempool conflicts here. Importantly, we want to ensure that transactions which were accepted using
    644+                // the below carve-out are able to be RBF'ed, without impacting the security the carve-out provides
    645+                // for off-chain contract systems (see link in the comment below).
    646+                //
    647+                // Specifically, the subset of RBF transactions which we allow despite conflicts are those which
    


    instagibbs commented at 2:21 pm on August 19, 2019:

    the subset of RBF transactions which we allow despite conflicts

    Aren’t RBF transactions conflicting by definition?

  31. in src/validation.cpp:660 in ab603961f1 outdated
    668+                }
    669+
    670+                for (const CTxIn& new_input: tx.vin) {
    671+                    if (conflict_existing_mempool_inputs.count(new_input.prevout.hash) == 0) {
    672+                        if (pool.exists(new_input.prevout.hash)) {
    673+                            return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", dummy_err_string);
    


    instagibbs commented at 2:28 pm on August 19, 2019:

    This check appears to be identical to https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L733

    which would at least be good to point out, or how it’s different, if it’s not the same. I could see the other check being relaxed in the future but this kept in place.


    TheBlueMatt commented at 6:47 pm on August 20, 2019:
    Indeed! Good call. I removes the entire block of code wholesale, but added extra comments to ensure if the BIP 125 rules are relaxed we know to re-add the checks here.
  32. in src/validation.cpp:631 in ab603961f1 outdated
    639+                // would meet the chain limits after the conflicts have been removed. However, there isn't a practical
    640+                // way to do this short of calculating the ancestor and descendant sets with an overlay cache of
    641+                // changed mempool entries. Due to both implementation and runtime complexity concerns, this isn't
    642+                // very realistic, either, thus we we try to ensure a limited set of transaction is RBF'able despite
    643+                // mempool conflicts here. Importantly, we want to ensure that transactions which were accepted using
    644+                // the below carve-out are able to be RBF'ed, without impacting the security the carve-out provides
    


    instagibbs commented at 2:30 pm on August 19, 2019:
    you should probably label the thing you’re calling carve-out for those not reading git history.
  33. in src/validation.cpp:641 in ab603961f1 outdated
    649+                // and which are not adding any new mempool dependencies (checked first).
    650+                // Such transactions are clearly not merging any existing packages, so we are only concerned with
    651+                // ensuring that (a) no package is growing past the package size (not count) limits and (b) we are
    652+                // not allowing something to effectively use the (below) carve-out spot when it shouldn't be allowed
    653+                // to.
    654+                // To check these we re-run CalculateMemPoolAncestors up to twice, once corresponding to the above,
    


    instagibbs commented at 2:33 pm on August 19, 2019:

    Having real trouble parsing this last paragraph.

    Lots of different things being referenced, “above”, “below”, “corresponding”, unsure exactly what it’s referring to.


    TheBlueMatt commented at 10:23 pm on August 19, 2019:
    Is this a bit more readable given the changes made for #16421#pullrequestreview-275251295 ?
  34. TheBlueMatt force-pushed on Aug 19, 2019
  35. TheBlueMatt force-pushed on Aug 19, 2019
  36. in src/validation.cpp:648 in 8b1884f38b outdated
    638+            // To check these we first check if we meet the RBF criteria, above, and increment the descendant
    639+            // limits by the single direct conflict (as these are recalculated by assuming the new transaction
    640+            // being added is a new dependant, with no removals, of each parents' existing dependant set).
    641+            // The ancestor count limits are unmodified (as the ancestor limits should be the same for both our
    642+            // new transaction and any conflicts).
    643+            assert(setIterConflicting.size() == 1);
    


    ajtowns commented at 1:19 am on August 20, 2019:
    setIterConflicting.size() could theoretically be zero if the conflicting hash from setConflicts isn’t in the mempool somehow. That shouldn’t be possible because we’ve held a lock on cs_main and pool.cs since we populated setConflicts, so this assertion should be correct.
  37. in src/validation.cpp:671 in 8b1884f38b outdated
    666         std::string errString;
    667         if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) {
    668             setAncestors.clear();
    669             // If CalculateMemPoolAncestors fails second time, we want the original error string.
    670             std::string dummy_err_string;
    671+            // Contracing/payment channels CPFP carve-out:
    


    ajtowns commented at 1:19 am on August 20, 2019:
    “Contracting” with a t? :)

    TheBlueMatt commented at 3:57 am on August 20, 2019:
    No one ever warned me I’d need to know how to spell to write code :(
  38. ajtowns approved
  39. ajtowns commented at 1:26 am on August 20, 2019: member
    ACK 8b1884f38bb7b4cd893ff13a2a86c1e975966711 - rechecked code, compiled and reran some tests, compared to previously acked commit.
  40. TheBlueMatt force-pushed on Aug 20, 2019
  41. TheBlueMatt force-pushed on Aug 20, 2019
  42. in test/functional/mempool_package_onemore.py:36 in b9dc087c99 outdated
    32@@ -33,7 +33,7 @@ def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs):
    33         outputs = {}
    34         for i in range(num_outputs):
    35             outputs[node.getnewaddress()] = send_value
    36-        rawtx = node.createrawtransaction(inputs, outputs)
    37+        rawtx = node.createrawtransaction(inputs, outputs, 0, True)
    


    instagibbs commented at 5:58 pm on August 30, 2019:
    named args would make this more obvious what’s going on without a comment
  43. instagibbs approved
  44. instagibbs commented at 6:23 pm on August 30, 2019: member

    ACK https://github.com/bitcoin/bitcoin/pull/16421/commits/b9dc087c99d327aaa194de31282fedde864b7740

    The new test also fails without the logic fix.

    0  File "./test/functional/mempool_package_onemore.py", line 86, in run_test
    1    self.nodes[0].sendrawtransaction(signed_second_tx['hex'])
    2  File "/home/instagibbs/elements-dev/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
    3    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
    4  File "/home/instagibbs/elements-dev/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__
    5    raise JSONRPCException(response['error'], status)
    6test_framework.authproxy.JSONRPCException: too-long-mempool-chain, too many descendants for tx 6a9a7075b6bfa6f2e13b064d9ace83cc2a941598a05546b4aa68d02b9f55db90 [limit: 25] (code 64) (-26)
    
  45. ajtowns commented at 4:27 am on September 2, 2019: member

    ACK b9dc087c99d327aaa194de31282fedde864b7740 ; changes since last ack are comments and dropping the “no new ancestors” check for this carve out; code review, checked compiled and tests pass.

    Dropping the checks seems correct – the original behaviour when conflicting with one tx should have been:

    0if (!has_new_ancestors && desc > LimitDesc+1) Invalid();
    1if (has_new_ancestors && desc > LimitDesc) Invalid();
    2if (has_new_ancestors) Invalid();
    

    and it should now be if (desc > LimitDesc+1 || has_new_ancestors) Invalid(); which looks correct to me. And there’s comments to document the dependency between the two bits of code, to help avoid introducing bugs in future.

  46. in src/validation.cpp:643 in b9dc087c99 outdated
    638+            // not allowing something to effectively use the (below) carve-out spot when it shouldn't be allowed
    639+            // to.
    640+            //
    641+            // To check these we first check if we meet the RBF criteria, above, and increment the descendant
    642+            // limits by the single direct conflict (as these are recalculated by assuming the new transaction
    643+            // being added is a new dependant, with no removals, of each parents' existing dependant set).
    


    sdaftuar commented at 6:57 pm on September 3, 2019:
    s/dependant/descendant/g s/parents’/parent’s/
  47. in src/validation.cpp:650 in b9dc087c99 outdated
    645+            // new transaction and any conflicts).
    646+            assert(setIterConflicting.size() == 1);
    647+            CTxMemPool::txiter conflict = *setIterConflicting.begin();
    648+
    649+            nLimitDescendants += 1;
    650+            nLimitDescendantSize += conflict->GetTxSize();
    


    sdaftuar commented at 3:52 pm on September 4, 2019:

    I believe we can make this conflict->GetSizeWithDescendants() and still respect our mempool limits.

    Without the size of the descendants, it’s possible that an attempt to RBF a single transaction could fail if there are descendants of that transaction in the mempool, due to the mempool limits (even though those child transactions would be replaced as well). By accounting for the size of those transactions, we should be able to accommodate any RBF attempt that conflicts with a single transaction and introduces no new mempool ancestors.


    instagibbs commented at 6:32 pm on September 4, 2019:

    TheBlueMatt commented at 7:53 pm on September 4, 2019:
    Done! good idea.

    ajtowns commented at 1:15 am on September 5, 2019:

    Can’t we actually cope with merging packages this way too though? If you’ve got a tx that has parents A, B, C; and that conflicts with tx’s X (parent A) and Y (parent B), then beforehand you had:

    0..., A, X, [children X]
    1..., B, Y, [children Y]
    

    (maybe some tx’s were descendants of both X and Y, but it’s worse if there weren’t any like that) and afterwards you have:

    0..., A, tx
    1..., B, tx
    

    You don’t have C in the mempool because that would fail the “replacement-adds-unconfirmed” test later.

    So you know tx’s ancestor checks pass, because they’re actually worked out; you know A,B’s ancestor checks pass because they don’t change, tx’s descendant check is trivial, and you know A,B and all their parent’s descendant checks passed, because they did so when X and Y were there – as far as sizes go, if they were all at their limit, then the max size for tx is the minimum of the descendant sizes of each tx that was replaced.

    So I think you could replace the setConflicts.size() == 1 test with:

    0if (!setConflicts.empty()) {
    1    auto conflict = setIterConflicting.begin();
    2    assert(conflict != setIterConflicting.end());
    3    uint64_t bump = (*conflict)->GetSizeWithDescendants();
    4    while(++conflict != setIterConflicting.end()) {
    5        bump = std::min(bump, (*conflict)->GetSizeWithDescendants());
    6    }
    7    nLimitDescendants += 1;
    8    nLimitDescendantSize += bump;
    9}
    

    Maybe I’m missing something though?


    TheBlueMatt commented at 6:01 pm on September 5, 2019:
    I agree there’s lots we can do in the future to improve things (not actually 100% sure on this one, these things are complicated…) lets try to land this as-is first, though, cause I do really want it for 0.19 still.
  48. sdaftuar commented at 5:50 pm on September 4, 2019: member
    I think this works, but we can make it slightly better. I was trying to think through how we’d describe the change here, and it seemed to me the explanation is “fee bumping a single transaction according to BIP 125 should work”. However, that is not quite the right explanation unless we factor in descendant size of the conflicting transaction, so I think we should go ahead and make that change.
  49. TheBlueMatt force-pushed on Sep 4, 2019
  50. Conservatively accept RBF bumps bumping one tx at the package limits
    Accept RBF bumps of single transactions (ie which conflict with one
    transaction) even when that transaction is a member of a package
    which is currently at the package limit iff the new transaction
    does not add any additional mempool dependencies from the original.
    
    This could be made a bit looser in the future and still be safe,
    but for now this fixes the case that a transaction which was
    accepted by the carve-out rule will not be directly RBF'able.
    5ce822efbe
  51. TheBlueMatt force-pushed on Sep 4, 2019
  52. TheBlueMatt commented at 7:54 pm on September 4, 2019: member
    Tweaked the increment to use GetSizeWithDescendants() instead and updated the comment (with some slight readability tweaks @instagibbs may appreciate).
  53. instagibbs commented at 7:58 pm on September 4, 2019: member

    re-ACK https://github.com/bitcoin/bitcoin/pull/16421/commits/5ce822efbe45513ce3517c1ca731ac6d6a0c3b54

    comment clarification and GetSizeWithDescendants change only.

  54. ajtowns commented at 1:16 am on September 5, 2019: member
    ACK 5ce822efbe45513ce3517c1ca731ac6d6a0c3b54 ; GetSizeWithDescendants is only change and makes sense
  55. sipa commented at 6:25 pm on September 6, 2019: member
    Code review ACK 5ce822efbe45513ce3517c1ca731ac6d6a0c3b54. I haven’t thought hard about the effect on potential DoS issues this policy change may have.
  56. fanquake referenced this in commit 0d20c42a01 on Sep 7, 2019
  57. fanquake merged this on Sep 7, 2019
  58. fanquake closed this on Sep 7, 2019

  59. fanquake removed this from the "Blockers" column in a project

  60. sdaftuar commented at 7:53 pm on September 9, 2019: member
    post-merge utACK
  61. DrahtBot locked this on Dec 16, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me