validation: followups for de-duplication of packages by glozow · Pull Request #23804 · bitcoin/bitcoin

glozow · 2021-12-17T16:15:47Z

This addresses some comments from review on e12fafd from #22674.

Improve documentation about de-duplication: comment
Fix code looking up same-txid-different-witness transaction in mempool: comment
Improve the interface for when a same-txid-different-witness transaction is swapped: comment
Add a test for witness swapping: comment
Add a test for packages with a mix of duplicate/different witness/new parents: comment
Fix issue with not notifying CValidationInterface when there's a partial submission due to fail-fast: comment

t-bast

Tiny nit about the test, but LGTM.
ACK fcc2f91

src/test/txpackage_tests.cpp

achow101 · 2021-12-17T18:30:41Z

ACK fcc2f91ed48b748c19cb99fad40f57de6cd3a445

ariard

ACK fcc2f91

Thanks for taking the comments from #22674!

jnewbery

I've left a few minor comments here, but there are a couple of more substantial comments in the original PR here: #22674 (review).

src/validation.cpp

src/test/txpackage_tests.cpp

src/validation.h

glozow · 2022-01-07T17:53:12Z

Addressed comments and added the fixups from @jnewbery's review on #22674 - most have no behavior changes so they're grouped in 1 commit. Another commit fixes the issue where we're not notifying MainSignals when there's a partial submission.

src/validation.h

t-bast

ACK 0300cb1

The previous interface required callers to guess that the tx had been swapped and look up the tx again by txid to find a `MEMPOOL_ENTRY` result. This is a confusing interface. Instead, explicitly tell the caller that this transaction was `DIFFERENT_WITNESS` in the result linked to the mempool entry's wtxid. This gives the caller all the information they need in 1 lookup, and they can query the mempool for the other transaction if needed.

If/when witness replacement is implemented in the future, this test case can be easily replaced with witness replacement tests.

No behavior changes, just clarifications.

Behavior change: don't quit right after LimitMempoolSize() when a package is partially submitted. We should still send TransactionAddedToMempool notifications for transactions that were submitted. Not behavior change: add a new package validation result for mempool logic errors.

glozow · 2022-01-17T12:44:26Z

Added more tests for funsies, the rest of the diff is just changing comments.

@ariard @achow101 @jnewbery @instagibbs @t-bast if you have time to take another look? Would be nice to add these fixups with #22674 before v23

achow101 · 2022-01-17T18:16:40Z

ACK 3cd7f69

t-bast

LGTM, ACK 3cd7f69

instagibbs

ACK 3cd7f69

ariard

ACK 3cd7f69

Substantial changes since my last ACK are new commits adding clarifications, improving the errors handling and extending the unit test.

ariard · 2022-01-25T01:56:00Z

src/validation.cpp

    LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(),
                     gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000,
                     std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)});
-    if (!all_submitted) return false;


Well technically if this code change now trigger a TransactionAddedToMempool(), I think we're changing the views of our CMainSignals consumers and that would constitute a behavior change. That said, if I'm correct, I don't think it's worthy to invalidate ACKs just to update commit description.

…ages 3cd7f69 [unit test] package parents are a mix (glozow) de075a9 [validation] better handle errors in SubmitPackage (glozow) 9d88853 AcceptPackage fixups (glozow) 2db77cd [unit test] different witness in package submission (glozow) 9ad211c [doc] more detailed explanation for deduplication (glozow) 83d4fb7 [packages] return DIFFERENT_WITNESS for same-txid-different-witness tx (glozow) Pull request description: This addresses some comments from review on e12fafd from bitcoin#22674. - Improve documentation about de-duplication: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770156708) - Fix code looking up same-txid-different-witness transaction in mempool: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770804029) - Improve the interface for when a same-txid-different-witness transaction is swapped: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770782822) - Add a test for witness swapping: [comment](https://github.com/bitcoin/bitcoin/pull/22674/files#r770804029) - Add a test for packages with a mix of duplicate/different witness/new parents: [comment](bitcoin#22674 (comment)) - Fix issue with not notifying `CValidationInterface` when there's a partial submission due to fail-fast: [comment](bitcoin#22674 (comment)) ACKs for top commit: achow101: ACK 3cd7f69 t-bast: LGTM, ACK bitcoin@3cd7f69 instagibbs: ACK 3cd7f69 ariard: ACK 3cd7f69 Tree-SHA512: a5d86ca86edab80a5a05fcbb828901c058b3f2fa2552912ea52f2871e29c3cf4cc34020e7aac2217959c9c3a01856f4bd3d631d844635b98144f212f76c2f3ef

ClaraBara22 · 2022-03-06T15:31:18Z

Can I comment?

Summary: This is a partial backport of [[bitcoin/bitcoin#23804 | core#23804]] bitcoin/bitcoin@9ad211c Test Plan: read comment Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12911

Summary: No behavior changes, just clarifications. This is a partial backport of [[bitcoin/bitcoin#23804 | core#23804]] bitcoin/bitcoin@9d88853 Depends on D12911 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12912

glozow mentioned this pull request Dec 17, 2021

validation: mempool validation and submission for packages of 1 child + parents #22674

Merged

DrahtBot added the Validation label Dec 17, 2021

t-bast approved these changes Dec 17, 2021

View reviewed changes

src/test/txpackage_tests.cpp Outdated Show resolved Hide resolved

src/test/txpackage_tests.cpp Outdated Show resolved Hide resolved

fanquake requested a review from ariard December 18, 2021 02:26

ariard reviewed Dec 20, 2021

View reviewed changes

jnewbery reviewed Jan 1, 2022

View reviewed changes

glozow force-pushed the 2021-12-dedup-packages branch from fcc2f91 to 0300cb1 Compare January 7, 2022 17:29

instagibbs reviewed Jan 10, 2022

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

t-bast approved these changes Jan 10, 2022

View reviewed changes

glozow added 5 commits January 17, 2022 12:17

[doc] more detailed explanation for deduplication

9ad211c

[unit test] different witness in package submission

2db77cd

If/when witness replacement is implemented in the future, this test case can be easily replaced with witness replacement tests.

AcceptPackage fixups

9d88853

No behavior changes, just clarifications.

glozow force-pushed the 2021-12-dedup-packages branch from 0300cb1 to fa5aafc Compare January 17, 2022 12:39

[unit test] package parents are a mix

3cd7f69

glozow force-pushed the 2021-12-dedup-packages branch from fa5aafc to 3cd7f69 Compare January 17, 2022 13:00

glozow mentioned this pull request Jan 17, 2022

Package Mempool Submission with Package Fee-Bumping #22290

Closed

10 tasks

t-bast approved these changes Jan 17, 2022

View reviewed changes

fanquake added this to the 23.0 milestone Jan 18, 2022

fanquake requested a review from instagibbs January 18, 2022 04:53

instagibbs approved these changes Jan 20, 2022

View reviewed changes

fanquake requested a review from darosior January 20, 2022 04:01

glozow requested a review from ariard January 21, 2022 12:10

ariard reviewed Jan 25, 2022

View reviewed changes

fanquake merged commit 417e750 into bitcoin:master Jan 25, 2022

glozow deleted the 2021-12-dedup-packages branch January 25, 2022 14:41

bitcoin locked and limited conversation to collaborators Mar 6, 2023

Conversation

glozow commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

achow101 commented Dec 17, 2021

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow commented Jan 7, 2022

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Jan 17, 2022

Uh oh!

achow101 commented Jan 17, 2022

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

ClaraBara22 commented Mar 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

glozow commented Dec 17, 2021 •

edited

Loading