refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

furszy · 2022-05-25T22:17:56Z

Based on a common function signature pattern that we have all around the sources:

bool doSomething(arg1, arg2, arg3, arg4, &result_obj, &error_string) {
    // do something...
    if (error) {
        error_string = "something bad happened";
        return false;
    }

    result = goodResult;
    return true;
}

Introduced a generic class BResult that encapsulate the function boolean result, the result object (in case of having it) and, in case of failure, the string error reason.

Obtaining in this way cleaner function signatures and removing boilerplate code:

BResult<Obj> doSomething(arg1, arg2, arg3, arg4) {
    // do something...
    if (error) return "something bad happened";

    return goodResult;
}

Same cleanup applies equally to the function callers' side as well. There is no longer need to add the error string and the result object declarations before calling the function:

Before:

Obj result_obj;
std::string error_string;
if (!doSomething(arg1, arg2, arg3, arg4, result_obj, error_string)) {
    LogPrintf("Error: %s", error_string);
}
return result_obj;

Now:

BResult<Obj> op_res = doSomething(arg1, arg2, arg3, arg4);
if (!op_res) {
    LogPrintf("Error: %s", op_res.GetError());
}
return op_res.GetObjResult();

Initial Implementation:

Have connected this new concept to two different flows for now:

The CreateTransaction flow. --> 7ba2b87
The GetNewDestination flow. --> bcee091

Happy note: even when introduced a new class into the sources, the amount of lines removed is almost equal to added ones :).

Extra note: this work is an extended version (and a decoupling) of the work that is inside #24845 (which does not contain the GetNewDestination changes nor the inclusion of the FeeCalculation field inside CreatedTransactionResult).

theStack · 2022-05-26T00:46:49Z

Concept ACK

DrahtBot · 2022-05-26T09:02:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25526 (wallet: avoid double keypool TopUp() calls on descriptor wallets by furszy)
#25297 (WIP, wallet: speedup transactions sync, rescan and load not flushing to db constantly by furszy)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
#25269 (wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error by furszy)
#25234 (bench: add benchmark for wallet 'AvailableCoins' function. by furszy)
#24897 ([Draft / POC] Silent Payments by w0xlt)
#23444 (fuzz: Add regression test for wallet crash by MarcoFalke)
#19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)

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.

furszy · 2022-05-27T12:57:43Z

rebased, conflicts solved.

w0xlt

Approach ACK

ryanofsky · 2022-06-16T19:37:15Z

This basically seems like a good design.

In the simple case, you call a function and it can either succeed or fail, and if it fails it will provide an error string. In this case, the function returns OperationResult which contains the information packaged up in a standard format.

In the slightly more complicated case, the function has a return value. Maybe it's an informational return value, or maybe it's a return value with details about errors if the caller needs to handle different errors in different ways. Either way, CallResult<T> can package up the result type with the error string, and return the information a standard format.

Some design questions / things I'm not sure about:

Can OperationResult bool m_res member be dropped? It seems like just having a single std::optional<bilingual_str> m_error member without the bool would provide a more minimal and unambiguous version of that class. Treating nullopt case as success, and non-nullopt case as error would avoid the need for the bool.
Maybe OperationResult type should be replaced with CallResult<void>, just have one result class instead of two classes
Maybe result classes should provide a standard way to chain errors. If some inner function returns an error string, the caller might want to add its own error string placing the error in context. I think we also have some wallet code that returns lists of warnings and lists of errors, so it's not clear if this result classes here would want to expand to handle this kind of thing, or they should just focus on simple cases.

No need to resolve these questions to merge the PR, I think. Can add result classes now, and experiment with using them, and add/remove features as needed.

laanwj · 2022-06-16T22:10:33Z

Concept ACK. I like this rust-inspired idea.
One additional idea might be to parametrize Result on both the result and error type (in case it is not always a string).

maflcko · 2022-06-17T06:30:14Z

I also wonder why this needs three members and 6 constructors. Can this be implemented in less code by just using a std::variant with two types?

Going to the extreme, with just two lines of code:

template <typename Obj, typename Err>
using CallResult = std::variant<Obj, Err>;

…everal code cleanups. fd5c996 wallet: GetAvailableBalance, remove double walk-through every available coin (furszy) 162d4ad wallet: add 'only_spendable' filter to AvailableCoins (furszy) cdf185c wallet: remove unused IsSpentKey(hash, index) method (furszy) 4b83bf8 wallet: avoid extra IsSpentKey -> GetWalletTx lookups (furszy) 3d8a282 wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n) (furszy) a06fa94 wallet: IsSpent, 'COutPoint' arg instead of (hash, index) (furszy) 91902b7 wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index) (furszy) 9472ca0 wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times (furszy) 4ce235e wallet: return 'CoinsResult' struct in `AvailableCoins` (furszy) Pull request description: This started in #24845< 8000 /a> but grew out of scope of it. So, points tackled: 1) Avoid extra `GetWalletTx` lookups inside `AvailableCoins -> IsSpentKey`. `IsSpentKey` was receiving the tx hash and index to internally lookup the tx inside the wallet's map. As all the `IsSpentKey` function callers already have the wtx available, them can provide the `scriptPubKey` directly. 2) Most of the time, we call `Wallet::AvailableCoins`, and later on the process, skip the non-spendable coins from the result in subsequent for-loops. So to speedup the process: introduced the ability to filter by "only_spendable" coins inside `Wallet::AvailableCoins` directly. (the non-spendable coins skip examples are inside `AttemptSelection->GroupOutputs` and `GetAvailableBalance`). 4) Refactored `AvailableCoins` in several ways: a) Now it will return a new struct `CoinsResult` instead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway). --> this is coming from #24845 but cherry-picked it here too to make the following commits look nicer. b) Unified all the 'wtx.tx->vout[I]' calls into a single call (coming from this comment #24699 (comment)). 5) The wallet `IsLockedCoin` and `IsSpent` methods now accept an `OutPoint` instead of a hash:index. Which let me cleanup a bunch of extra code. 6) Speeded up the wallet 'GetAvailableBalance': filtering `AvailableCoins` by spendable outputs only and using the 'AvailableCoins' retrieved `total_amount` instead of looping over all the retrieved coins once more. ------------------------------------------------------- Side topic, all this process will look even nicer with #25218 ACKs for top commit: achow101: ACK fd5c996 brunoerg: crACK fd5c996 w0xlt: Code Review ACK fd5c996 Tree-SHA512: 376a85476f907f4f7d1fc3de74b3dbe159b8cc24687374d8739711ad202ea07a33e86f4e66dece836da3ae6985147119fe584f6e672f11d0450ba6bd165b3220

furszy · 2022-07-08T12:27:12Z

Thanks @w0xlt for the review!

The CI has an error, but the error message does not match the current PR code.
wallet.GetNewDestination(OutputType::BECH32, "", destination, error); was replaced in 26288ac

Good eye. Seems that it's a silent conflict with master due #24924 merge. Going to rebase it once more.

furszy · 2022-07-08T13:56:15Z

All green now, ready to go.

Useful to encapsulate the function result object (in case of having it) or, in case of failure, the failure reason. This let us clean lot of boilerplate code, as now instead of returning a boolean and having to add a ref arg for the return object and another ref for the error string. We can simply return a 'BResult<Obj>'. Example of what we currently have: ``` bool doSomething(arg1, arg2, arg3, arg4, &result, &error_string) { do something... if (error) { error_string = "something bad happened"; return false; } result = goodResult; return true; } ``` Example of what we will get with this commit: ``` BResult<Obj> doSomething(arg1, arg2, arg3, arg4) { do something... if (error) return {"something happened"}; // good return {goodResult}; } ``` This allows a similar boilerplate cleanup on the function callers side as well. They don't have to add the extra pre-function-call error string and result object declarations to pass the references to the function.

…Result'

…nRef>

furszy · 2022-07-08T14:24:23Z

Rebased once more to include the recently merged #25337 here as well.

w0xlt

reACK 111ea3a

theStack

re-ACK 111ea3a

achow101 · 2022-07-11T17:44:27Z

ACK 111ea3a

maflcko

review ACK 111ea3a 🎏

Show signature

Signature:

src/util/result.h

maflcko · 2022-07-12T11:02:32Z

src/util/result.h

+    bool HasRes() const { return std::holds_alternative<T>(m_variant); }
+
+    /* In case of success, the result object */
+    const T& GetObj() const {


might be good to add a mutable getter, so that the result can be de-capsulated by the caller

Edit: Fixed in #25594

Also LIFETIMEBOUND?

maflcko · 2022-07-12T11:03:34Z

src/util/result.h

+    }
+
+    /* In case of failure, the error cause */
+    const bilingual_str& GetError() const {


Shouldn't this be annotated with LIFETIMEBOUND as it is returning a pointer to internal memory?

Diff to reproduce use-after-free with asan:

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index fda56ccff7..2abf2db298 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -4,6 +4,7 @@ #include <util/system.h> +#include <util/result.h> #include <clientversion.h> #include <fs.h> #include <hash.h> // For Hash() @@ -2659,6 +2660,13 @@ BOOST_AUTO_TEST_CASE(message_hash) BOOST_CHECK_NE(message_hash1, signature_hash); } +BOOST_AUTO_TEST_CASE(bresult) +{ + const auto& long_str{"Looooooooooooooooooooooooooooooooooooooooooooooooong string"}; + const auto& err{BResult<int>{Untranslated(long_str)}.GetError()}; + BOOST_CHECK_EQUAL(err.original, long_str); +} + BOOST_AUTO_TEST_CASE(remove_prefix) { BOOST_CHECK_EQUAL(RemovePrefix("./util/system.h", "./"), "util/system.h");

With the warning added:

diff --git a/src/util/result.h b/src/util/result.h index dcf5edaa5b..18364b3c6e 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -6,6 +6,8 @@ #define BITCOIN_UTIL_RESULT_H #include <util/translation.h> +#include <attributes.h> + #include <variant> /* @@ -32,7 +34,7 @@ public: } /* In case of failure, the error cause */ - const bilingual_str& GetError() const { + const bilingual_str& GetError() const LIFETIMEBOUND { assert(!HasRes()); return std::get<bilingual_str>(m_variant); }

Results in:

test/util_tests.cpp:2666:19: warning: temporary bound to local reference 'err' will be destroyed at the end of the full-expression [-Wdangling] const auto& err{BResult<int>{Untranslated(long_str)}.GetError()}; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated.

src/wallet/spend.h

maflcko · 2022-07-12T11:10:44Z

src/wallet/spend.cpp

@@ -964,7 +962,7 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal(
              feeCalc.est.fail.start, feeCalc.est.fail.end,
              (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) : 0.0,
              feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool);
-    return CreatedTransactionResult(tx, nFeeRet, nChangePosInOut);
+    return CreatedTransactionResult(tx, nFeeRet, nChangePosInOut, feeCalc);


Could use {} to clarify that this is not a function and to enable narrowing warnings for the int types.

@MarcoFalke Use {} where? You mean CreatedTransactionResult{tx, nFeeRet, nChangePosInOut, feeCalc} instead of current version?

maflcko · 2022-07-12T11:16:23Z

src/qt/walletmodel.cpp


        auto& newTx = transaction.getWtx();
-        newTx = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired, error);
+        const auto& res = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired);


while touching this line: clang-tidy named args are of the format /*sign=*/

maflcko · 2022-07-12T11:34:42Z

src/wallet/spend.cpp

-        std::optional<CreatedTransactionResult> txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, error2, tmp_cc, sign);
+        auto res_tx_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign);
+        // Helper optional class for now
+        std::optional<CreatedTransactionResult> txr_grouped{res_tx_grouped.HasRes() ? std::make_optional(res_tx_grouped.GetObj()) : std::nullopt};


nit could use std::move here, if the mutable BResult interface is implemented

maflcko · 2022-07-12T11:39:26Z

src/bench/wallet_loading.cpp

+    const auto& dest = wallet.GetNewDestination(OutputType::BECH32, "");
+    assert(dest.HasRes());


nit: Could write shorter as const CTxDestination& dest{Assert(wallet.GetNewDestination(OutputType::BECH32, "")).GetObj()};

Actually nvm. This would likely be a use-after-free.

Maybe there could be a ReleaseObj helper to obtain the ownership?

…onnect …

Riahiamirreza · 2022-07-14T17:02:13Z

src/util/result.h

+ * (in case of success) or propagating the error cause.
+*/
+template<class T>
+class BResult {


What does that B means at the beginning of class name?

Comes from "Bilingual". The story behind it is in the last paragraph of this comment: #25218 (comment)

Riahiamirreza · 2022-07-14T18:20:41Z

src/wallet/spend.cpp

@@ -964,7 +962,7 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal(
              feeCalc.est.fail.start, feeCalc.est.fail.end,
              (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) : 0.0,
              feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool);
-    return CreatedTransactionResult(tx, nFeeRet, nChangePosInOut);
+    return CreatedTransactionResult(tx, nFeeRet, nChangePosInOut, feeCalc);


@MarcoFalke Use {} where? You mean CreatedTransactionResult{tx, nFeeRet, nChangePosInOut, feeCalc} instead of current version?

Riahiamirreza · 2022-07-15T11:37:01Z

I think this PR can have Good first review label. For someone like me which is very new to the project, understanding the concept and implementation of this PR was easy (relative to other PRs).

maflcko · 2022-08-02T12:59:12Z

There is a follow-up in case anyone wants to review it: #25721

…om `GetReservedDestination` methods 76b3c37 refactor: wallet: return util::Result from `GetReservedDestination` methods (Sebastian Falbesoner) Pull request description: This PR is a follow-up to #25218, as suggested in comment bitcoin/bitcoin#25218 (comment). The interfaces of the methods `ReserveDestination::GetReservedDestination`, `{Legacy,Descriptor,}ScriptPubKeyMan::GetReservedDestination` are improved by returning `util::Result<CTxDestination>` instead of `bool` in order to get rid of the two `CTxDestination&` and `bilingual_str&` out-parameters. ACKs for top commit: furszy: ACK 76b3c37 Tree-SHA512: bf15560a88d645bcf8768024013d36012cd65caaa4a613e8a055dfd8f29cb4a219c19084606992bad177920cdca3a732ec168e9b9526f9295491f2cf79cc6815

Summary: > Useful to encapsulate the function result object (in case of having it) or, in case of failure, the failure reason. > > This let us clean lot of boilerplate code, as now instead of returning a boolean and having to add a ref arg for the > return object and another ref for the error string. We can simply return a 'BResult<Obj>'. > > Example of what we currently have: > ``` > bool doSomething(arg1, arg2, arg3, arg4, &result, &error_string) { > do something... > if (error) { > error_string = "something bad happened"; > return false; > } > > result = goodResult; > return true; > } > ``` > > Example of what we will get with this commit: > ``` > BResult<Obj> doSomething(arg1, arg2, arg3, arg4) { > do something... > if (error) return {"something happened"}; > > // good > return {goodResult}; > } > ``` > > This allows a similar boilerplate cleanup on the function callers side as well. They don't have to add the extra > pre-function-call error string and result object declarations to pass the references to the function. bitcoin/bitcoin@7a45c33 ---- > Prepare BResult for non-copyable types bitcoin/bitcoin@fa8de09 ---- > Refactor: Replace BResult with util::Result > > Rename `BResult` class to `util::Result` and update the class interface to be > more compatible with `std::optional` and with a full-featured result class > implemented in bitcoin/bitcoin#25665. Motivation for > this change is to update existing `BResult` usages now so they don't have to > change later when more features are added in #25665. > > This change makes the following improvements originally implemented in #25665: > > - More explicit API. Drops potentially misleading `BResult` constructor that > treats any bilingual string argument as an error. Adds `util::Error` > constructor so it is never ambiguous when a result is being assigned an error > or non-error value. > > - Better type compatibility. Supports `util::Result<bilingual_str>` return > values to hold translated messages which are not errors. > > - More standard and consistent API. `util::Result` supports most of the same > operators and methods as `std::optional`. `BResult` had a less familiar > interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj > naming was also not internally consistent. > > - Better code organization. Puts `src/util/` code in the `util::` namespace so > naming reflects code organization and it is obvious where the class is coming > from. Drops "B" from name because it is undocumented what it stands for > (bilingual?) > > - Has unit tests. bitcoin/bitcoin@a23cca5 ---- > util: Add void support to util::Result > > A minimal (but hacky) way to add support for void to Result > originally posted bitcoin/bitcoin#27632 (comment) bitcoin/bitcoin@5f49cb1 ---- This is a partial backport of [[bitcoin/bitcoin#25218 | core#25218]], [[bitcoin/bitcoin#25594 | core#25594]], [[bitcoin/bitcoin#25721 | core#25721]] and [[bitcoin/bitcoin#25977 | core#25977]] The wallet use cases in the first 3 pull requests are not yet applicable to Bitcoin ABC due to missing dependencies. The `std::optional<bilingual_str>` use cases from [[bitcoin/bitcoin#25977 | core#25977]] will be backported in their own diff. Test Plan: `ninja && ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D14991

furszy changed the title ~~wallet: introduce generic 'Result' classes and connect it to CreateTransaction and GetNewDestination~~ wallet: introduce generic 'Result' classes and connect it to CreateTransaction and GetNewDestination May 25, 2022

furszy force-pushed the 2022_generic_result branch from 914e095 to a19f1dc Compare May 25, 2022 22:32

DrahtBot added the Refactoring label May 25, 2022

furszy changed the title ~~wallet: introduce generic 'Result' classes and connect it to CreateTransaction and GetNewDestination~~ refactor: introduce generic 'Result' classes and connect it to CreateTransaction and GetNewDestination May 26, 2022

This was referenced May 26, 2022

tidy: use modernize-use-raw-string-literal #25110

Closed

tracing: fix coin_selection:aps_create_tx_internal calling logic #25003

Merged

wallet: do not count wallet utxos as external #24649

Merged

fuzz: Add regression test for wallet crash #23444

Closed

DrahtBot added the Needs rebase label May 26, 2022

DrahtBot mentioned this pull request May 26, 2022

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

furszy force-pushed the 2022_generic_result branch from a19f1dc to d9ffeab Compare May 27, 2022 12:57

furszy force-pushed the 2022_generic_result branch from d9ffeab to 043714c Compare May 27, 2022 13:23

DrahtBot removed the Needs rebase label May 27, 2022

furszy changed the title ~~refactor: introduce generic 'Result' classes and connect it to CreateTransaction and GetNewDestination~~ refactor: introduce generic 'Result' classes and connect them to CreateTransaction and GetNewDestination May 28, 2022

This was referenced May 28, 2022

bench: add benchmark for wallet 'AvailableCoins' function. #25234

Merged

wallet: re-activate "AmountWithFeeExceedsBalance" error #25269

Open

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Merged

DrahtBot mentioned this pull request Jun 4, 2022

[Draft / POC] Silent Payments #24897

Closed

w0xlt reviewed Jun 11, 2022

View reviewed changes

furszy mentioned this pull request Jun 15, 2022

wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005

Merged

DrahtBot added the Needs rebase label Jun 16, 2022

achow101 mentioned this pull request Jun 18, 2022

rpc: add minconf/maxconf options to sendall and fund transaction calls #25375

Merged

DrahtBot removed the Needs rebase label Jun 20, 2022

furszy force-pushed the 2022_generic_result branch from 26288ac to 2ad9f01 Compare July 8, 2022 12:31

furszy added 4 commits July 8, 2022 11:18

wallet: refactor, include 'FeeCalculation' inside 'CreatedTransaction…

198fcca

…Result'

send: refactor CreateTransaction flow to return a BResult<CTransactio…

2235172

…nRef>

wallet: refactor GetNewDestination, use BResult

111ea3a

furszy force-pushed the 2022_generic_result branch from 2ad9f01 to 111ea3a Compare July 8, 2022 14:23

w0xlt 8000 approved these changes Jul 8, 2022

View reviewed changes

theStack approved these changes Jul 9, 2022

View reviewed changes

maflcko approved these changes Jul 12, 2022

View reviewed changes

maflcko merged commit 316afb1 into bitcoin:master Jul 12, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 12, 2022

Merge bitcoin#25218: refactor: introduce generic 'Result' class and c…

8b89538

…onnect …

ryanofsky mentioned this pull request Jul 13, 2022

util: Make BResult error a generic type instead of only bilingual_str #25601

Closed

Riahiamirreza reviewed Jul 14, 2022

View reviewed changes

daira mentioned this pull request Jul 20, 2022

Define a "result" type for use in C++ error handling zcash/zcash#4816

Closed

theStack mentioned this pull request Jul 20, 2022

refactor: wallet: return util::Result from GetReservedDestination methods #25656

Merged

ryanofsky mentioned this pull request Jul 27, 2022

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

furszy mentioned this pull request Jul 28, 2022

wallet: return error msg for "too-long-mempool-chain" #24845

Merged

maflcko mentioned this pull request Aug 1, 2022

refactor: Replace BResult with util::Result #25721

Merged

furszy mentioned this pull request Aug 9, 2022

wallet: Check max transaction weight in CoinSelection #25729

Merged

furszy mentioned this pull request Apr 3, 2023

Mark "too-long-mempool-chain error msg" completed NYDIG/bitcoin-task-bounty#32

Merged

furszy deleted the 2022_generic_result branch May 27, 2023 01:51

bitcoin locked and limited conversation to collaborators May 26, 2024

		const auto& dest = wallet.GetNewDestination(OutputType::BECH32, "");
		assert(dest.HasRes());

refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

Uh oh!

Conversation

Uh oh!

Initial Implementation:

Uh oh!

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!