wallet, mempool: propagete `checkChainLimits` error message to wallet #28863

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.

TheCharlatan

ACK bd5417e

src/interfaces/chain.h

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

Thanks for review @TheCharlatan @furszy

Addressed @furszy comment
Forced pushed bd5417e to 28115d8 Compare diff

BrandonOdiwuor

ACK 28115d8

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)

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

src/wallet/spend.cpp

-            return util::Error{_("Transaction has too long of a mempool chain")};
+        auto result = wallet.chain().checkChainLimits(tx);
+        if (!result) {
+            return util::Error{util::ErrorString(result)};


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.

Update CheckPackageLimits to use util::Result to pass the error message instead of out parameter. Also update test to reflect the error message from `CTxMempool` `CheckPackageLimits` output.

Sjors

utACK 8dec9c5

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

src/txmempool.h

@@ -605,11 +605,9 @@ class CTxMemPool
     *                                          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

TheCharlatan

Re-ACK 8dec9c5

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

utACK 8dec9c5

instagibbs

LGTM

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

src/txmempool.h

@@ -605,11 +605,9 @@ class CTxMemPool
     *                                          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

TheCharlatan approved these changes Nov 23, 2023

View reviewed changes

DrahtBot requested review from Sjors and BrandonOdiwuor 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 TheCharlatan November 24, 2023 19:39

TheCharlatan reviewed Nov 24, 2023

View reviewed changes

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

DrahtBot requested a review from TheCharlatan 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

TheCharlatan reviewed Nov 25, 2023

View reviewed changes

DrahtBot mentioned this pull request Dec 7, 2023

[WIP] Cluster mempool implementation #28676

Draft

8 tasks

wallet, mempool: propagete checkChainLimits error message to wallet

8dec9c5

Update CheckPackageLimits to use util::Result to pass the error message instead of out parameter. Also update test to reflect the error message from `CTxMempool` `CheckPackageLimits` output.

ismaelsadeeq force-pushed the 11-2023-prograte-checkPackageLimit-error-up branch from e572a53 to 8dec9c5 Compare December 17, 2023 20:14

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 TheCharlatan 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

TheCharlatan 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
16 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet, mempool: propagete `checkChainLimits` error message to wallet #28863

wallet, mempool: propagete `checkChainLimits` error message to wallet #28863

ismaelsadeeq commented Nov 13, 2023 •

edited

DrahtBot commented Nov 13, 2023 •

edited

Sjors left a comment

ismaelsadeeq commented Nov 14, 2023 •

edited

BrandonOdiwuor left a comment •

edited

BrandonOdiwuor left a comment

Sjors commented Nov 17, 2023

ismaelsadeeq commented Nov 17, 2023

TheCharlatan left a comment

ismaelsadeeq commented Nov 24, 2023

BrandonOdiwuor left a comment

ismaelsadeeq commented Nov 24, 2023

TheCharlatan Nov 25, 2023

ismaelsadeeq Nov 25, 2023

Sjors Dec 18, 2023

Sjors left a comment

Sjors Dec 18, 2023

instagibbs Dec 18, 2023

ismaelsadeeq Dec 18, 2023

ismaelsadeeq Dec 19, 2023

Sjors commented Dec 18, 2023

TheCharlatan left a comment

glozow commented Dec 18, 2023

instagibbs left a comment

instagibbs Dec 18, 2023

wallet, mempool: propagete checkChainLimits error message to wallet #28863

wallet, mempool: propagete checkChainLimits error message to wallet #28863

Conversation

ismaelsadeeq commented Nov 13, 2023 • edited

DrahtBot commented Nov 13, 2023 • edited

Code Coverage

Reviews

Conflicts

Sjors left a comment

Choose a reason for hiding this comment

ismaelsadeeq commented Nov 14, 2023 • edited

BrandonOdiwuor left a comment • edited

Choose a reason for hiding this comment

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Sjors commented Nov 17, 2023

ismaelsadeeq commented Nov 17, 2023

TheCharlatan left a comment

Choose a reason for hiding this comment

ismaelsadeeq commented Nov 24, 2023

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

ismaelsadeeq commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Dec 18, 2023

TheCharlatan left a comment

Choose a reason for hiding this comment

glozow commented Dec 18, 2023

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallet, mempool: propagete `checkChainLimits` error message to wallet #28863

wallet, mempool: propagete `checkChainLimits` error message to wallet #28863

ismaelsadeeq commented Nov 13, 2023 •

edited

DrahtBot commented Nov 13, 2023 •

edited

ismaelsadeeq commented Nov 14, 2023 •

edited

BrandonOdiwuor left a comment •

edited