test: clean and extend availablecoins

furszy commented at 7:00 PM on August 5, 2022: member

Negative PR with extended test coverage :).

Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result.
The class AvailableCoinsTestingSetup inside availablecoins_tests.cpp is a plain copy of ListCoinsTestingSetup that is inside wallet_tests.cpp.

So, deleted the file and moved the BasicOutputTypesTest test case to wallet_tests.cpp.
Added arg to include/skip locked coins from the AvailableCoins result. This is needed for point (1) as otherwise the wallet will spend the coins that we recently created due its closeness to the recipient amount. Note: this last point comes from #25659 where I'm using the same functionality to clean/speedup another flow as well.

fanquake added the label Wallet on Aug 5, 2022

DrahtBot commented at 4:18 AM on August 6, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, aureleoules, achow101

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:

#26756 (wallet: Replace GetBalance() logic with AvailableCoins() by w0xlt)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
#25659 (wallet: simplify ListCoins implementation by furszy)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction 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.

DrahtBot added the label Needs rebase on Aug 17, 2022

furszy force-pushed on Aug 18, 2022

furszy commented at 1:27 PM on August 18, 2022: member

rebased, conflicts solved.

furszy force-pushed on Aug 18, 2022

DrahtBot removed the label Needs rebase on Aug 18, 2022

in src/wallet/test/wallet_tests.cpp:636 in d799db5b87 outdated

 629 | +    util::Result<CTxDestination> dest = Assert(context.wallet->GetNewDestination(out_type, ""));
 630 | +    CWalletTx& wtx = context.AddTx(CRecipient{{GetScriptForDestination(*dest)}, amount, /*fSubtractFeeFromAmount=*/true});
 631 | +    CoinsResult available_coins = AvailableCoins(*context.wallet, nullptr, std::nullopt, 1, MAX_MONEY, MAX_MONEY, 0 , true, false);
 632 | +    // Lock outputs so they are not spent in follow-up transactions
 633 | +    for (uint32_t i = 0; i < wtx.tx->vout.size(); i++) context.wallet->LockCoin({wtx.GetHash(), i});
 634 | +    for (const auto& it : available_coins.coins) BOOST_CHECK_EQUAL(it.second.size(), expected_coins_sizes[it.first]);

aureleoules commented at 3:17 PM on November 1, 2022:

    for (const auto& [output_type, coins] : available_coins.coins) BOOST_CHECK_EQUAL(coins.size(), expected_coins_sizes[output_type]);

furszy commented at 11:53 PM on December 13, 2022:

tackled. Thanks.

aureleoules approved

aureleoules commented at 3:17 PM on November 1, 2022: member

ACK d799db5b87462243c685e04a91c128e1b24128ab - LGTM

DrahtBot added the label Needs rebase on Nov 16, 2022

wallet: AvailableCoins, add arg to include/skip locked coins 212ccdf2c2

furszy force-pushed on Nov 16, 2022

DrahtBot removed the label Needs rebase on Nov 16, 2022

aureleoules approved

aureleoules commented at 2:45 PM on November 17, 2022: member

reACK 6ef8af2a7d7b202f4ae9b7490434a86eb4eb143f

theStack commented at 5:01 PM on December 7, 2022: contributor

Concept ACK

One suggestion (feel free to ignore, as there is already an ACK): I think the changes in the second commit would be much easier to review if the deduplication refactors and the additional test coverage ("coverage for 'AvailableCoins' incremental result") are not mixed up and are split into two individual commits instead.

furszy commented at 11:04 PM on December 7, 2022: member

Thanks for the review!

One suggestion (feel free to ignore, as there is already an ACK): I think the changes in the second commit would be much easier to review if the deduplication refactors and the additional test coverage ("coverage for 'AvailableCoins' incremental result") are not mixed up and are split into two individual commits instead.

The "incremental result" coverage just means we are now checking that previous coins are still on the available coins vector too (before, we were merely checking that the new ones appear there). It was actually a good side-effect of the cleanup and code improvements. I don't think that is something that worth to split-up (plus, it would probably mean more code into the first commit, not less).

in src/wallet/test/availablecoins_tests.cpp:69 in 1e34836712 outdated

  64 | +    CoinFilterParams filter;
  65 | +    filter.skip_locked = false;
  66 | +    CoinsResult available_coins = AvailableCoins(*context.wallet, nullptr, std::nullopt, filter);
  67 | +    // Lock outputs so they are not spent in follow-up transactions
  68 | +    for (uint32_t i = 0; i < wtx.tx->vout.size(); i++) context.wallet->LockCoin({wtx.GetHash(), i});
  69 | +    for (const auto& it : available_coins.coins) BOOST_CHECK_EQUAL(it.second.size(), expected_coins_sizes[it.first]);

theStack commented at 11:15 PM on December 13, 2022:

As shortly discussed via IRC, the test currently also succeeds if filter.skip_locked is set to true. That is because we only iterate output types for which coins exist in available_coins. Should be fixed by iterating over all output types here.

furszy commented at 11:18 PM on December 13, 2022:

yes, good catch!

furszy force-pushed on Dec 13, 2022

furszy commented at 11:52 PM on December 13, 2022: member

Updated per feedback. Flipped the expected size checks. Small Diff

theStack commented at 2:04 PM on December 14, 2022: contributor

Updated per feedback. Flipped the expected size checks. Small Diff

Thanks, that solved the issue. Running the test with filter.skip_locked set to true fails now as expected:

wallet/test/wallet_tests.cpp(636): error: in "wallet_tests/BasicOutputTypesTest": check size == available_coins.coins[type].size() has failed [2 != 0]

Just one organizational nit: seems like this latest change is unintentionally part of the last commit (which is supposed to be purely move-only) rather than in the second one?

test: extend and simplify availablecoins_tests

Clean redundant code and add coverage for 'AvailableCoins' incremental result.

f69347d058

test: move coins result test to wallet_tests.cpp

The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy
of `ListCoinsTestingSetup` that is inside wallet_tests.cpp.

9622fe64b8

furszy force-pushed on Dec 14, 2022

furszy commented at 2:17 PM on December 14, 2022: member

Just one organizational nit: seems like this latest change is unintentionally part of the last commit (which is supposed to be purely move-only) rather than in the second one?

Pushed, thanks.

theStack approved

theStack commented at 11:36 AM on December 16, 2022: contributor

ACK 9622fe64b8785430c71d4abc8637075026dc690c

Verified that the commits deduplicate code in a nice way and increase test coverage at the same time. I don't have a strong opinion about the last commit, since I'm not sure if decreasing the number of compilation units is necessarily better (-> longer compilation time if only changes in BasicOutputTypesTest are made), and if we can be sure that AvailableCoinsTestingSetup will never differ from ListCoinsTestingSetup. Absolutely not a blocker, but would still be interesting to know what the original motivation was to put this test in an extra file with its own test setup and if it still applies (@josibake).

furszy requested review from aureleoules on Dec 21, 2022

aureleoules approved

aureleoules commented at 1:30 PM on December 22, 2022: member

reACK 9622fe64b8785430c71d4abc8637075026dc690c, nice cleanup!

maflcko assigned achow101 on Dec 22, 2022

achow101 commented at 5:46 PM on January 3, 2023: member

ACK 9622fe64b8785430c71d4abc8637075026dc690c

achow101 merged this on Jan 3, 2023

achow101 closed this on Jan 3, 2023

sidhujag referenced this in commit 3c49a7de2a on Jan 4, 2023

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024

test: clean and extend availablecoins_tests coverage #25789

Reviews

Conflicts