Similar to #32958 and motivated by #32958#pullrequestreview-3107029707, these changes
TransactionError to TransactionResponseTransactionErrorString to TransactionResultStringRPCErrorFromTransactionError to RPCErrorFromTransactionResponseThe reason is that TransactionError has TransactionError::Ok, which means that it is not always an error. The correct naming would instead be TransactionResponse
TransactionError contains the result TransactionError::Ok which can be
confusing because that is not an error but an Ok response. This updates
TransactionError to instead be named TransactionResult which is more
accurate.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Approach NACK | janb84 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
maxfeerate wallet startup option by ismaelsadeeq)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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible typos and grammar issues:
package topology disallowed. not child-with-parents or parents depend on each other. -> package topology disallowed; not child-with-parents, or parents depend on each other. [English is awkward/broken: starts a new sentence with lowercase “not” after a period, making the intended structure hard to read without guessing.]Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):
return BroadcastTransaction(*m_context, std::move(tx), err_string, max_tx_fee, broadcast_method, /*wait_callback=*/false); in src/node/interfaces.cppconst TransactionResponse tx_res = BroadcastTransaction(m_node, tx, err_string, max_tx_fee, broadcast_method, /*wait_callback=*/false); in src/node/interfaces.cppconst TransactionResponse err = BroadcastTransaction(node, tx, err_string, max_raw_tx_fee, method, /*wait_callback=*/true); in src/rpc/mempool.cpp... = BroadcastTransaction(... /*max_tx_fee=*/0, node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL, /*wait_callback=*/true); in src/rpc/mempool.cpp<sup>2026-04-18 14:18:56</sup>
Approach NACK
I do not think this is the best future proof way to accomplish the goal. In C++ 23 std::expected is introduced, which is a better way (imo) to do for explicit error handling. E.g link link2
Yes we are currently on C++ 20 but a "polyfill" std::expected is already introduced in the code as util::Expected<T, E>
This makes the upgrade path to C++23 an easy search and replace and is a better way to do explicit error handling / remove the ok from the errors.