wallet, mempool: propagete `checkChainLimits` error message to wallet by ismaelsadeeq · Pull Request #28863 · bitcoin/bitcoin

ismaelsadeeq · 2023-11-13T16:57:33Z

Requested in #28391 comment
The error message is static when a new transaction is created and package limit is reached.
Transaction has too long of a mempool chain
While the CTxMempool::CheckPackageLimits provide explicit information about the error message.
This PR updates CTxMempool::CheckPackageLimits return type to util::Result<void>, CheckPackageLimits now returns void when package limit is not hit, and returns the error string whenever package limit is hit instead of using out parameter errString.
The PR updates checkChainLimits return type to util::Result<void>.
Now the wallet CreateTransactionInternal will have access to the package limit error string whenever its hit.
Also Updated functional test to reflect the error message from CTxMempool::CheckPackageLimits output.

DrahtBot · 2023-11-13T16:57:36Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, TheCharlatan, glozow
Stale ACK	BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29086 (refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition by luke-jr)
#28676 ([WIP] Cluster mempool implementation 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.

Sjors

Concept ACK.

It would be nice if the test can reach all four possible limit error messages, but if that's difficult to achieve it could wait for a followup.

test/functional/wallet_basic.py

ismaelsadeeq · 2023-11-14T18:04:23Z

It would be nice if the test can reach all four possible limit error messages, but if that's difficult to achieve it could wait for a followup.

Previously we always got the Transaction has too long of a mempool chain message but with this PR we now get the node unconfirmed parents limit.
checkChainLimits is sending a package with one Internally created transaction and its vsize as the package vsize, package count will always be one.

BrandonOdiwuor

Concept ACK

using the error message from checkChainLimits(...)

BrandonOdiwuor

The PR looks good, left a Nit comment

src/node/interfaces.cpp

Sjors · 2023-11-17T17:46:04Z

Note that with cluster mempool #28676 the error would change. Which is another reason why I think it's useful to have tests cases for all four scenario's. But it can wait for another PR, and perhaps @sdaftuar could write it :-)

ismaelsadeeq · 2023-11-17T17:59:15Z

Thanks @Sjors
I Would also like to work on it whenever that happens.

sedited

ACK bd5417e47cd2452817d7de859839b63210233a5c

src/interfaces/chain.h

ismaelsadeeq · 2023-11-24T19:13:49Z

Thanks for review @TheCharlatan @furszy

Addressed @furszy comment
Forced pushed bd5417e47cd2452817d7de859839b63210233a5c to 28115d81b2 Compare diff

BrandonOdiwuor

ACK 28115d81b2eacb15dd4835d5536a71b0b5ac835b

src/node/interfaces.cpp

ismaelsadeeq · 2023-11-24T22:17:39Z

Force pushed from 28115d8 to e572a53
Compare diff

Updated the PR OP and title
Addressed @TheCharlatan comment wallet, mempool: propagete checkChainLimits error message to wallet #28863 (comment)

sedited · 2023-11-25T09:59:18Z

src/wallet/spend.cpp

Just a comment: Would be nice to get #25665 merged, so we can get a move constructor here.

Thanks @TheCharlatan that can come as a follow-up post #25665 I think?

Indeed, or it could even be done as part of that PR. It depends on which one gets merged first.

Sjors

utACK 8dec9c5

Sjors · 2023-12-18T09:44:12Z

src/txmempool.h

     *                                          to mempool. The transactions need not be direct
     *                                          ancestors/descendants of each other.
     * @param[in]       total_vsize             Sum of virtual sizes for all transactions in package.
-     * @param[out]      errString               Populated with error reason if a limit is hit.


* @returns {} or the error reason if a limit is hit.

this would be good

Thanks Would create a tiny follow-up to fix this, so as not to invalidate ACK's.

This is fixed in #29115

Sjors · 2023-12-18T09:49:55Z

cc @glozow & @instagibbs

sedited

Re-ACK 8dec9c5

glozow · 2023-12-18T10:27:20Z

utACK 8dec9c5

instagibbs

LGTM

instagibbs · 2023-12-18T14:26:21Z

src/txmempool.h

     *                                          to mempool. The transactions need not be direct
     *                                          ancestors/descendants of each other.
     * @param[in]       total_vsize             Sum of virtual sizes for all transactions in package.
-     * @param[out]      errString               Populated with error reason if a limit is hit.


this would be good

…eLimits` returns 19bb65b [doc]: add doxygen return comment for CheckPackageLimits (ismaelsadeeq) Pull request description: This PR adds a doxygen comment on `CheckPackageLimits` describing what the method returns. Fixes #28863 (comment) ACKs for top commit: Sjors: utACK 19bb65b Zero-1729: utACK 19bb65b Tree-SHA512: ccf1cc00a44d3fff60f28ad6766019a9f61b349729eab3cb02bc76b13c2e55441348a1602d806e60e4b2eabeb1f5d1ddacddf86c0bcdb78b078bb3a863b650c2

DrahtBot added the Wallet label Nov 13, 2023

Sjors reviewed Nov 14, 2023

View reviewed changes

test/functional/wallet_basic.py Outdated Show resolved Hide resolved

ismaelsadeeq force-pushed the 11-2023-prograte-checkPackageLimit-error-up branch from 2b4e86f to bd5417e Compare November 14, 2023 16:47

BrandonOdiwuor reviewed Nov 15, 2023

View reviewed changes

BrandonOdiwuor reviewed Nov 16, 2023

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

sedited approved these changes Nov 23, 2023

View reviewed changes

DrahtBot requested review from BrandonOdiwuor and Sjors November 23, 2023 21:47

furszy reviewed Nov 23, 2023

View reviewed changes

src/interfaces/chain.h Outdated Show resolved Hide resolved

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor November 23, 2023 22:17

ismaelsadeeq force-pushed the 11-2023-prograte-checkPackageLimit-error-up branch from bd5417e to 28115d8 Compare November 24, 2023 19:09

BrandonOdiwuor approved these changes Nov 24, 2023

View reviewed changes

DrahtBot requested a review from sedited November 24, 2023 19:39

sedited reviewed Nov 24, 2023

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from sedited November 24, 2023 20:59

ismaelsadeeq force-pushed the 11-2023-prograte-checkPackageLimit-error-up branch from 28115d8 to e572a53 Compare November 24, 2023 22:00

ismaelsadeeq changed the title ~~wallet: propagete checkChainLimits error message to wallet~~ wallet, mempool: propagete checkChainLimits error message to wallet Nov 24, 2023

This was referenced Nov 25, 2023

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

policy: nVersion=3 and Package RBF #25038

Closed

sedited reviewed Nov 25, 2023

View reviewed changes

DrahtBot mentioned this pull request Dec 7, 2023

Cluster mempool implementation #28676

Closed

8 tasks

DrahtBot removed the Needs rebase label Dec 17, 2023

DrahtBot mentioned this pull request Dec 18, 2023

refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition #29086

Merged

Sjors approved these changes Dec 18, 2023

View reviewed changes

DrahtBot requested review from BrandonOdiwuor and sedited and removed request for BrandonOdiwuor December 18, 2023 09:49

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor December 18, 2023 09:49

sedited approved these changes Dec 18, 2023

View reviewed changes

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor December 18, 2023 09:54

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor December 18, 2023 10:27

instagibbs approved these changes Dec 18, 2023

View reviewed changes

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor December 18, 2023 14:27

fanquake assigned glozow Dec 18, 2023

glozow merged commit dd39194 into bitcoin:master Dec 18, 2023

glozow removed their assignment Dec 18, 2023

ismaelsadeeq mentioned this pull request Dec 19, 2023

[doc]: add doxygen comment describing what CheckPackageLimits returns #29115

Merged

ismaelsadeeq deleted the 11-2023-prograte-checkPackageLimit-error-up branch June 27, 2024 11:16

bitcoin locked and limited conversation to collaborators Jun 27, 2025

Conversation

ismaelsadeeq commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ismaelsadeeq commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrandonOdiwuor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Nov 17, 2023

Uh oh!

ismaelsadeeq commented Nov 17, 2023

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ismaelsadeeq commented Nov 24, 2023

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ismaelsadeeq commented Nov 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Dec 18, 2023

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Dec 18, 2023

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ismaelsadeeq commented Nov 13, 2023 •

edited

Loading

DrahtBot commented Nov 13, 2023 •

edited

Loading

ismaelsadeeq commented Nov 14, 2023 •

edited

Loading

BrandonOdiwuor left a comment •

edited

Loading