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.

Sign up for GitHub

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

Jump to bottom

mempool clean up: replace update_* structs with lambdas #26048

Merged
merged 1 commit into from Sep 12, 2022
Merged

mempool clean up: replace update_* structs with lambdas #26048

merged 1 commit into from Sep 12, 2022

Conversation

glozow
Copy link
Member

@glozow glozow commented Sep 8, 2022

These were introduced in commit 5add7a7, when the codebase was pre-C++11. We can use lambdas now.

maflcko
maflcko reviewed Sep 8, 2022
View reviewed changes
src/txmempool.cpp Outdated Show resolved Hide resolved
amovfx
amovfx reviewed Sep 9, 2022
View reviewed changes
Copy link

@amovfx amovfx left a comment

Choose a reason for hiding this comment

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

Just trying to help out here. I pulled it down and it built fine of course. But I don't really have anything to really run to hit the code path. I assume that the tests that are already available do so.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 9, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25673 (refactor: make member functions const when applicable by aureleoules)
  • #23962 (Use int32_t type for transaction size/weight consistently by hebasto)

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.

@glozow
[mempool] replace update_descendant_state with lambda
1b348d2 
These were introduced in commit 5add7a7, when the codebase was
pre-C++11. They are no longer necessary.
@glozow
Copy link
Member Author

glozow commented Sep 9, 2022 •
edited

@amovfx probably the best way to review this PR is to review the code manually and check that no behavior has changed. If you want to run a quick smoke test (would show if something is really wrong in mempool entry accounting), I often use make check && test/functional/test_runner.py test/functional/mempool* test/functional/rpc_packages.py test/functional/feature_rbf.py test/functional/mining_prioritisetransaction.py.

@amovfx
Copy link

amovfx commented Sep 9, 2022 •
edited

@amovfx probably the best way to review this PR is to review the code manually and check that no behavior has changed. If you want to run a quick smoke check (would show if something is really wrong in mempool entry accounting), I often use make check && test/functional/test_runner.py test/functional/mempool* test/functional/rpc_packages.py test/functional/feature_rbf.py test/functional/mining_prioritisetransaction.py.

Thanks! Ill do this. I did this and everything is working, I tried running the test thats getting caught on the check for 32-bit + dash [gui] [CentOS 8], "wallet_reorgsrestore.py" and my test passed fine on macOS.

I imagine I should be trying to test that in a docker container?

w0xlt
w0xlt approved these changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

ACK 1b348d2

@maflcko
Copy link
Member

maflcko commented Sep 12, 2022

review ACK 1b348d2 👮

Show signature

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 1b348d2725f6271d7f78b4668ab35014cdb176be 👮
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiypwwAgyoDOkuAp46McbdxAuSLZzb+fsg8fERzK2j86FthtnBmh4UVwpcPYMcf
6/egvnEKi2vN+3lafv0S6wKouxp9X+vkEOfz/USygH29rgyq425DWlwHRwHO1H5l
Ea2bE/5bVWoVf4Zj6tF+8WGhecIMKJhHZ+KmHcBihcVFT79yHDI05sBdofFnBZmz
ig7DH4+xj7zfUzRRocpadEPtfkNsastbquyXEFpagI4kCUCVDJ2PL5DK4+5Zy8PL
nZARtu+DMBOiWZkCm6eUCtGsa9SzwCACY/bf93dFU/i80kMDNCw/UXp0Arva3LVm
kp6yGodkDQQGUHzMPIox7F8zhgppOvvNTZs7p0VxFsGN7o3M5zV/MMaaTb+afc8i
txOeBT3O5sWzKQ5SDqOR1+kBFDXkOZMcWjpDFsrq8HbnRKXusN1dFKt96UOaAdGC
BOcFRVf7oJBtdcibDNXIsr7Vv6OMnxAdvNlXvoc/4DBhE0q8errN7DyvLh1JvFnm
wJEf9iGR
=9yhH
-----END PGP SIGNATURE-----

@maflcko maflcko merged commit 5558d2f into bitcoin:master Sep 12, 2022
jonatack
jonatack reviewed Sep 12, 2022
View reviewed changes
src/txmempool.cpp
@@ -146,7 +114,9 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan
modifyCount++;
cachedDescendants[updateIt].insert(mapTx.iterator_to(descendant));
// Update ancestor state for each descendant
mapTx.modify(mapTx.iterator_to(descendant), update_ancestor_state(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCost()));
mapTx.modify(mapTx.iterator_to(descendant), [=](CTxMemPoolEntry& e) {
e.UpdateAncestorState(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCost());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
e.UpdateAncestorState(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCost());
e.UpdateAncestorState(updateIt->GetTxSize(), updateIt->GetModifiedFee(), /*modifyCount=*/1, updateIt->GetSigOpCost());

jonatack
jonatack reviewed Sep 12, 2022
View reviewed changes
src/txmempool.cpp
@@ -393,7 +363,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
CAmount modifyFee = -removeIt->GetModifiedFee();
int modifySigOps = -removeIt->GetSigOpCost();
for (txiter dit : setDescendants) {
mapTx.modify(dit, update_ancestor_state(modifySize, modifyFee, -1, modifySigOps));
mapTx.modify(dit, [=](CTxMemPoolEntry& e){ e.UpdateAncestorState(modifySize, modifyFee, -1, modifySigOps); });
Copy link
Contributor

@jonatack jonatack Sep 12, 2022 •
edited

Choose a reason for hiding this comment

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

Suggested change
mapTx.modify(dit, [=](CTxMemPoolEntry& e){ e.UpdateAncestorState(modifySize, modifyFee, -1, modifySigOps); });
mapTx.modify(dit, [=](CTxMemPoolEntry& e) { e.UpdateAncestorState(modifySize, modifyFee, /*modifyCount=*/-1, modifySigOps); });

@glozow glozow deleted the 2022-09-mempool-cleanup branch September 12, 2022 10:30
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 12, 2022
Merge bitcoin#26048: mempool clean up: replace update_* structs with …
604c76b 
…lambdas

1b348d2 [mempool] replace update_descendant_state with lambda (glozow)

Pull request description:

  These were introduced in commit bitcoin@5add7a7, when the codebase was pre-C++11. We can use lambdas now.

ACKs for top commit:
  MarcoFalke:
    review ACK 1b348d2 👮
  w0xlt:
    ACK bitcoin@1b348d2

Tree-SHA512: b664425b395e39ecf1cfc1e731200378261cf58c3985075fdc6027731a5caf995de72ea25be99b4c0dbec2e3ee6cf940e7c577638844619c66c8494ead5da459
@bitcoin bitcoin locked and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonatack jonatack jonatack left review comments

@maflcko maflcko maflcko left review comments

@amovfx amovfx amovfx left review comments

@w0xlt w0xlt w0xlt approved these changes

Assignees
No one assigned
Labels
Mempool Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@glozow @DrahtBot @amovfx @maflcko @jonatack @w0xlt