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

ismaelsadeeq commented at 4:57 pm on November 13, 2023: member

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 commented at 4:57 pm on November 13, 2023: contributor

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.

DrahtBot added the label Wallet on Nov 13, 2023

in test/functional/wallet_basic.py:615 in 2b4e86f79a outdated

611@@ -612,10 +612,11 @@ def run_test(self):
612         # So we should be able to generate exactly chainlimit txs for each original output
613         sending_addr = self.nodes[1].getnewaddress()
614         txid_list = []
615-        for _ in range(chainlimit * 2):
616+        chain_limit_2x = 2 * chainlimit

Sjors commented at 6:19 am on November 14, 2023:

nit: I don’t think adding this variable improves readability

ismaelsadeeq commented at 4:47 pm on November 14, 2023:

Thanks Fixed

Sjors commented at 6:21 am on November 14, 2023: member

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.

ismaelsadeeq force-pushed on Nov 14, 2023

ismaelsadeeq commented at 6:04 pm on November 14, 2023: member

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 commented at 10:20 am on November 15, 2023: contributor

Concept ACK

using the error message from checkChainLimits(...)

in src/node/interfaces.cpp:61 in bd5417e47c outdated

57@@ -58,6 +58,7 @@
58 #include <any>
59 #include <memory>
60 #include <optional>
61+#include <string>

BrandonOdiwuor commented at 2:32 pm on November 16, 2023:

Nit: I believe this addition might be unnecessary, considering that other instances of std::string in the file seem to work fine without it

maflcko commented at 2:38 pm on November 16, 2023:

According to iwyu, it seems better to include

BrandonOdiwuor commented at 2:34 pm on November 16, 2023: contributor

The PR looks good, left a Nit comment

Sjors commented at 5:46 pm on November 17, 2023: member

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 commented at 5:59 pm on November 17, 2023: member

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

TheCharlatan approved

TheCharlatan commented at 9:47 pm on November 23, 2023: contributor

ACK bd5417e47cd2452817d7de859839b63210233a5c

DrahtBot requested review from Sjors on Nov 23, 2023

DrahtBot requested review from BrandonOdiwuor on Nov 23, 2023

in src/interfaces/chain.h:263 in bd5417e47c outdated

259@@ -260,7 +260,7 @@ class Chain
260     virtual void getPackageLimits(unsigned int& limit_ancestor_count, unsigned int& limit_descendant_count) = 0;
261 
262     //! Check if transaction will pass the mempool's chain limits.
263-    virtual bool checkChainLimits(const CTransactionRef& tx) = 0;
264+    virtual bool checkChainLimits(const CTransactionRef& tx, std::string& err_string) = 0;

furszy commented at 10:17 pm on November 23, 2023:

Not blocking. Could return util::Result<void> and inline all the err_string return messages within the CheckPackageLimits function.

ismaelsadeeq commented at 7:11 pm on November 24, 2023:

Thanks @furszy updated to your suggestion, its more elegant, and no need for out parameter err_string

DrahtBot removed review request from BrandonOdiwuor on Nov 23, 2023

DrahtBot requested review from BrandonOdiwuor on Nov 23, 2023

DrahtBot removed review request from BrandonOdiwuor on Nov 23, 2023

DrahtBot requested review from BrandonOdiwuor on Nov 23, 2023

ismaelsadeeq force-pushed on Nov 24, 2023

ismaelsadeeq commented at 7:13 pm on November 24, 2023: member

Thanks for review @TheCharlatan @furszy

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

BrandonOdiwuor approved

BrandonOdiwuor commented at 7:39 pm on November 24, 2023: contributor

ACK 28115d81b2eacb15dd4835d5536a71b0b5ac835b

DrahtBot requested review from TheCharlatan on Nov 24, 2023

in src/node/interfaces.cpp:709 in 28115d81b2 outdated

707         LockPoints lp;
708         CTxMemPoolEntry entry(tx, 0, 0, 0, 0, false, 0, lp);
709         LOCK(m_node.mempool->cs);
710         std::string err_string;
711-        return m_node.mempool->CheckPackageLimits({tx}, entry.GetTxSize(), err_string);
712+        if (m_node.mempool->CheckPackageLimits({tx}, entry.GetTxSize(), err_string)) return {};

TheCharlatan commented at 8:59 pm on November 24, 2023:

Since we’re now changing interfaces, I think CheckPackageLimits could return a Result type too.

ismaelsadeeq commented at 10:01 pm on November 24, 2023:

Taken in https://github.com/bitcoin/bitcoin/pull/28863/commits/e572a530d224214121371c9e42a013ef3b26ee0e Thanks

DrahtBot requested review from TheCharlatan on Nov 24, 2023

ismaelsadeeq force-pushed on Nov 24, 2023

ismaelsadeeq renamed this:
~~wallet: propagete `checkChainLimits` error message to wallet~~
wallet, mempool: propagete `checkChainLimits` error message to wallet
on Nov 24, 2023

ismaelsadeeq commented at 10:17 pm on November 24, 2023: member

Force pushed from https://github.com/bitcoin/bitcoin/commit/28115d81b2eacb15dd4835d5536a71b0b5ac835b to https://github.com/bitcoin/bitcoin/commit/e572a530d224214121371c9e42a013ef3b26ee0e Compare diff

Updated the PR OP and title
Addressed @TheCharlatan comment #28863 (review)

in src/wallet/spend.cpp:1301 in e572a530d2 outdated

1250@@ -1251,8 +1251,9 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1251 
1252     if (gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {
1253         // Lastly, ensure this tx will pass the mempool's chain limits
1254-        if (!wallet.chain().checkChainLimits(tx)) {
1255-            return util::Error{_("Transaction has too long of a mempool chain")};
1256+        auto result = wallet.chain().checkChainLimits(tx);
1257+        if (!result) {
1258+            return util::Error{util::ErrorString(result)};

TheCharlatan commented at 9:59 am on November 25, 2023:

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

ismaelsadeeq commented at 7:33 pm on November 25, 2023:

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

Sjors commented at 9:00 am on December 18, 2023:

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

DrahtBot added the label Needs rebase on Dec 14, 2023

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

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.

8dec9c560b

ismaelsadeeq force-pushed on Dec 17, 2023

DrahtBot removed the label Needs rebase on Dec 17, 2023

in src/txmempool.h:608 in 8dec9c560b

604@@ -605,11 +605,9 @@ class CTxMemPool
605      *                                          to mempool. The transactions need not be direct
606      *                                          ancestors/descendants of each other.
607      * @param[in]       total_vsize             Sum of virtual sizes for all transactions in package.
608-     * @param[out]      errString               Populated with error reason if a limit is hit.

Sjors commented at 9:44 am on December 18, 2023:

0* [@returns](/bitcoin-bitcoin/contributor/returns/) {} or the error reason if a limit is hit.

instagibbs commented at 2:26 pm on December 18, 2023:

this would be good

ismaelsadeeq commented at 2:32 pm on December 18, 2023:

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

ismaelsadeeq commented at 4:20 pm on December 19, 2023:

This is fixed in #29115

Sjors approved

Sjors commented at 9:49 am on December 18, 2023: member

utACK 8dec9c560b53488c1e71d8f74241c7dce42cb387