wallet: group outputs only once, decouple it from Coin Selection #25806

pull furszy wants to merge 7 commits into bitcoin:master from furszy:2022_wallet_single_outputs_grouping_process changing 9 files +476 −149
  1. furszy commented at 9:19 pm on August 8, 2022: member

    The idea originates from #24845 (comment).

    Note: For clarity, it’s recommended to start reviewing from the end result to understand the structure of the flow.

    GroupOutputs function rationale:

    If “Avoid Partial Spends” is enabled, the function gathers outputs with the same script together inside a container. So Coin Selection can treats them as if them were just one possible input and either select them all or not select them.

    How the Inputs Fetch + Selection process roughly works:

     01. Fetch users manually selected inputs.
     12. Fetch wallet available coins (walks through the entire wallet txes map) and insert them into a set of vectors (each vector store outputs from a single type).
     23. Coin Selection Process:
     3   Call `AttemptSelection` 8 times. Each of them expands the coin eligibility filter (accepting a larger subset of coins in the calculation) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.
     4
     5   Each `AttemptSelection` call performs the following actions:
     6     - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and a combination of all of them):
     7       Call ChooseSelectionResult providing the respective, filtered by type, coins vector. Which:
     8           I. Groups the outputs vector twice (one for positive only and a second one who includes the negative ones as well).
     9              - GroupOutputs walks-through the entire inputted coins vector one time at least, + more if we are avoiding partial spends, to generate a vector of OutputGroups.
    10           II. Then performs every coin selection algorithm using the recently created vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
    11           III. Then returns the best solution out of them.
    

    We perform the general operation of gathering outputs, with the same script, into a single container inside: Each coins selection attempt (8 times —> each coin eligibility filter), for each of the outputs vector who were filtered by type (plus another one joining all the outputs as well if needed), twice (one for the positive only outputs effective value and a second one for all of them).

    So, in the worst case scenario where no solution is found after the 8 Coin Selection attempts, the GroupOutputs function is called 80 times (8 * 5 * 2).

    Improvements:

    This proposal streamlines the process so that the output groups, filtered by coin eligibility and type, are created in a single loop outside of the Coin Selection Process.

    The new process is as follows:

     01. Fetch user’s manually selected inputs.
     12. Fetch wallet available coins.
     23. Group outputs by each coin eligibility filter and each different output type found.
     34. Coin Selection Process: 
     4   Call AttemptSelection 8 times. Each of them expands the coin eligibility filter (accepting different output groups) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.
     5   
     6   Each ‘AttemptSelection’ call performs the following actions:
     7      - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and all of them):
     8          A. Call ‘ChooseSelectionResult’ providing the respective, filtered by type, output group. Which:
     9             I. Performs every coin selection algorithm using the provided vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
    10             II. Then returns the best solution out of them.
    

    Extra Note: The next steps after this PR will be to:

    1. Merge AvailableCoins and GroupOutputs processes.
    2. Skip entire coin selection rounds if no new coins are added into the subsequent round.
    3. Remove global feerates from the OutputGroup class.
    4. Remove secondary “grouped” tx creation from CreateTransactionInternal by running Coin Selection results over the aps grouped outputs vs non-aps ones.
  2. fanquake added the label Wallet on Aug 8, 2022
  3. DrahtBot commented at 5:46 am on August 9, 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 S3RK, theStack, achow101
    Concept ACK Xekyo

    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:

    • #26720 (wallet: coin selection, don’t return results that exceed the max allowed weight by furszy)
    • #25982 (wallet: Guard against undefined behaviour by yancyribbens)
    • #25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
    • #24897 ([Draft / POC] Silent Payments by w0xlt)
    • #23475 (wallet: add config to prioritize a solution that doesn’t create change in coin selection by brunoerg)

    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.

  4. DrahtBot added the label Needs rebase on Aug 17, 2022
  5. furszy renamed this:
    wallet: single outputs grouping calculation
    wallet: single group outputs calculation, decouple it from Coin Selection
    on Sep 6, 2022
  6. furszy renamed this:
    wallet: single group outputs calculation, decouple it from Coin Selection
    wallet: single group outputs calculation, decoupled from Coin Selection
    on Sep 6, 2022
  7. furszy force-pushed on Oct 27, 2022
  8. furszy renamed this:
    wallet: single group outputs calculation, decoupled from Coin Selection
    wallet: group outputs only once, decouple it from Coin Selection
    on Oct 27, 2022
  9. DrahtBot removed the label Needs rebase on Oct 27, 2022
  10. furszy force-pushed on Nov 16, 2022
  11. in src/wallet/test/group_outputs_tests.cpp:64 in ae41a8b0f4 outdated
    60+
    61+ CoinSelectionParams makeSelectionParams(bool avoid_partial_spends)
    62+{
    63+    FastRandomContext rand{};
    64+    return CoinSelectionParams{
    65+            rand,
    


    aureleoules commented at 11:33 pm on November 16, 2022:
    CoinSelectionParams’s rng_fast member is a reference and you’re using a stack object, this will create a dangling reference.

    furszy commented at 2:22 pm on November 17, 2022:
    oops, good catch 👍🏼
  12. in src/wallet/coinselection.h:259 in ae41a8b0f4 outdated
    255+    // Map filters to output groups.
    256+    std::map<OutputType, Groups> groups_by_type;
    257+    // All groups, no filters
    258+    Groups all_groups;
    259+
    260+    enum InsertGroupType {
    


    aureleoules commented at 11:34 pm on November 16, 2022:
    prefer enum class
  13. in src/wallet/spend.cpp:428 in ae41a8b0f4 outdated
    440-            group.Insert(output, ancestors, descendants, positive_only);
    441-
    442-            // Check the OutputGroup's eligibility. Only add the eligible ones.
    443-            if (positive_only && group.GetSelectionAmount() <= 0) continue;
    444-            if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups_out.push_back(group);
    445+        for (const auto& coins_by_type : coins.coins) {
    


    aureleoules commented at 11:35 pm on November 16, 2022:
    could use structured binding
  14. in src/wallet/spend.cpp:503 in ae41a8b0f4 outdated
    503 
    504-    // Now we go through the entire map and pull out the OutputGroups
    505-    for (const auto& spk_and_groups_pair: spk_to_groups_map) {
    506-        const std::vector<OutputGroup>& groups_per_spk= spk_and_groups_pair.second;
    507+    // Now we go through the entire maps and pull out the OutputGroups
    508+    const auto& push_output_groups = [&](ScriptPubKeyToOutgroup& groups_map ,bool positive_only) {
    


    aureleoules commented at 11:36 pm on November 16, 2022:
    0    const auto& push_output_groups = [&](const ScriptPubKeyToOutgroup& groups_map ,bool positive_only) {
    
  15. in src/wallet/spend.cpp:504 in ae41a8b0f4 outdated
    504-    // Now we go through the entire map and pull out the OutputGroups
    505-    for (const auto& spk_and_groups_pair: spk_to_groups_map) {
    506-        const std::vector<OutputGroup>& groups_per_spk= spk_and_groups_pair.second;
    507+    // Now we go through the entire maps and pull out the OutputGroups
    508+    const auto& push_output_groups = [&](ScriptPubKeyToOutgroup& groups_map ,bool positive_only) {
    509+        for (const auto& spk_and_groups_pair: groups_map) {
    


    aureleoules commented at 11:36 pm on November 16, 2022:
    could use structured binding
  16. in src/wallet/spend.cpp:562 in ae41a8b0f4 outdated
    560-    return std::nullopt;
    561+    return util::Error();
    562 };
    563 
    564-std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins, const CoinSelectionParams& coin_selection_params)
    565+util::Result<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, Groups& groups,
    


    aureleoules commented at 11:37 pm on November 16, 2022:
    wallet is unused

    furszy commented at 2:23 pm on November 17, 2022:
    good eye, this let me remove the AttemptSelection wallet arg too.
  17. in src/wallet/spend.cpp:670 in ae41a8b0f4 outdated
    673-            }
    674-            if (auto r5{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2),
    675-                                   available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) {
    676-                return r5;
    677-            }
    678+            ordered_filters.emplace_back(CoinEligibilityFilter(0, 1, 2));
    


    aureleoules commented at 11:38 pm on November 16, 2022:

    No need for temporary object, same around this line.

    0            ordered_filters.emplace_back(0, 1, 2);
    

    furszy commented at 2:26 pm on November 17, 2022:
    I actually didn’t make this change on purpose to minimize the diff burden inside the same commit. Would prefer to do it in a different commit to make reviewers’ life a bit easier (adding the args names there too).
  18. in src/wallet/spend.cpp:978 in ae41a8b0f4 outdated
    957@@ -925,7 +958,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
    958     // behavior."
    959     const uint32_t nSequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL};
    960     for (const auto& coin : selected_coins) {
    961-        txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence));
    962+        txNew.vin.push_back(CTxIn(coin->outpoint, CScript(), nSequence));
    


    aureleoules commented at 11:39 pm on November 16, 2022:

    could use emplace_back

    0        txNew.vin.emplace_back(coin->outpoint, CScript(), nSequence);
    

    furszy commented at 2:27 pm on November 17, 2022:
    agree but it’s unrelated to the commit changes. Let’s better leave it for another PR.
  19. in src/wallet/test/group_outputs_tests.cpp:44 in ae41a8b0f4 outdated
    40+    tx.vout[0].nValue = nValue;
    41+    tx.vout[0].scriptPubKey = dest;
    42+
    43+    const uint256& txid = tx.GetHash();
    44+    LOCK(wallet.cs_wallet);
    45+    auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{}));
    


    aureleoules commented at 11:40 pm on November 16, 2022:
    could use structured binding
  20. furszy force-pushed on Nov 17, 2022
  21. furszy force-pushed on Nov 17, 2022
  22. furszy force-pushed on Nov 17, 2022
  23. furszy force-pushed on Nov 17, 2022
  24. DrahtBot added the label Needs rebase on Dec 6, 2022
  25. furszy force-pushed on Dec 6, 2022
  26. DrahtBot removed the label Needs rebase on Dec 6, 2022
  27. fanquake commented at 5:23 pm on December 6, 2022: member
    PR description needs be updated to remove the “this is built on”, for the already-merged PR. Although, given Footnote; this PR is still a WIP: in the PR description, maybe this should also be a draft (that would better communicate the WIP intent than a footnote in description)? If it’s no-longer WIP, please update the PR description to remove that as well. Can be done along with the next rebase.
  28. DrahtBot added the label Needs rebase on Dec 6, 2022
  29. furszy commented at 6:19 pm on December 6, 2022: member

    PR description needs be updated to remove the “this is built on”, for the already-merged PR.

    removed, thanks.

    Although, given Footnote; this PR is still a WIP: in the PR description, maybe this should also be a draft (that would better communicate the WIP intent than a footnote in description)? If it’s no-longer WIP, please update the PR description to remove that as well. Can be done along with the next rebase.

    yeah. This one is on my current working-path. I’m planning to update the PR description soon. Since yesterday, have been working on #26560 (review) which will make this PR a bit smaller.

  30. fanquake marked this as a draft on Dec 8, 2022
  31. furszy force-pushed on Dec 23, 2022
  32. furszy force-pushed on Dec 23, 2022
  33. furszy force-pushed on Dec 23, 2022
  34. furszy force-pushed on Dec 23, 2022
  35. DrahtBot removed the label Needs rebase on Dec 23, 2022
  36. furszy force-pushed on Dec 23, 2022
  37. furszy force-pushed on Dec 26, 2022
  38. furszy force-pushed on Dec 27, 2022
  39. furszy force-pushed on Dec 27, 2022
  40. achow101 referenced this in commit 3f8591d46b on Jan 3, 2023
  41. furszy force-pushed on Jan 4, 2023
  42. furszy marked this as ready for review on Jan 4, 2023
  43. sidhujag referenced this in commit cab438784e on Jan 4, 2023
  44. in src/wallet/coinselection.h:276 in 325f171145 outdated
    272+    // and/or the 'all groups' container.
    273+    void push(const OutputGroup& group, OutputType type, InsertGroupType insert_type);
    274+    // Retrieves 'Groups' filtered by type
    275+    std::optional<Groups> find(OutputType type);
    276+    // Different output types count
    277+    int typesCount() { return groups_by_type.size(); }
    


    aureleoules commented at 3:26 pm on January 19, 2023:

    nit:

    0    size_t typesCount() const { return groups_by_type.size(); }
    
  45. in src/wallet/test/group_outputs_tests.cpp:26 in 6df2b735b6 outdated
    21+    wallet->LoadWallet();
    22+    LOCK(wallet->cs_wallet);
    23+    wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
    24+    wallet->SetupDescriptorScriptPubKeyMans();
    25+    return wallet;
    26+}
    


    aureleoules commented at 3:44 pm on January 19, 2023:

    Somewhat unrelated, it would be nice to extract this function in test utils.

    0grep -FnrIi 'std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());' src/wallet/test
    1src/wallet/test/coinselector_tests.cpp:306:        std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());
    2src/wallet/test/coinselector_tests.cpp:329:        std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());
    3src/wallet/test/coinselector_tests.cpp:352:        std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());
    4src/wallet/test/coinselector_tests.cpp:417:    std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());
    5src/wallet/test/coinselector_tests.cpp:727:    std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());
    6src/wallet/test/coinselector_tests.cpp:749:    std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());
    7src/wallet/test/coinselector_tests.cpp:1058:    std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());
    8src/wallet/test/group_outputs_tests.cpp:20:    std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase());
    

    furszy commented at 1:58 am on January 20, 2023:
    Great catch! Cleaned inside #26720 (comment).
  46. in src/wallet/coinselection.cpp:421 in 325f171145 outdated
    420-    for (const COutput& coin : inputs) {
    421-        waste += coin.GetFee() - coin.long_term_fee;
    422-        selected_effective_value += use_effective_value ? coin.GetEffectiveValue() : coin.txout.nValue;
    423+    for (const auto& coin : inputs) {
    424+        waste += coin->GetFee() - coin->long_term_fee;
    425+        selected_effective_value += use_effective_value ? coin->GetEffectiveValue() : coin->txout.nValue;
    


    aureleoules commented at 3:52 pm on January 19, 2023:

    nit: Could avoid some dereferences.

    0        const COutput& out = *coin;
    1        waste += out.GetFee() - out.long_term_fee;
    2        selected_effective_value += use_effective_value ? out.GetEffectiveValue() : out.txout.nValue;() : out.txout.nValue;
    

    furszy commented at 1:22 pm on January 20, 2023:
    done
  47. in src/wallet/spend.cpp:527 in 62ed8f6a23 outdated
    520@@ -521,8 +521,11 @@ util::Result<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmo
    521 {
    522     // Run coin selection on each OutputType and compute the Waste Metric
    523     std::vector<SelectionResult> results;
    524-    for (const auto& it : available_coins.coins) {
    525-        auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, it.second, coin_selection_params)};
    526+    for (const auto& [type, coins] : available_coins.coins) {
    527+        Groups groups;
    528+        groups.positive_group = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, true /* positive_only */);
    529+        groups.mixed_group = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, false /* positive_only */);
    


    aureleoules commented at 4:03 pm on January 19, 2023:

    nit

    0        groups.positive_group = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, /*positive_only=*/true );
    1        groups.mixed_group = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, /*positive_only=*/false );
    

    furszy commented at 1:18 pm on January 20, 2023:
    This lines were just moved from one place to another (tried to not change them to make the review easier), and are later removed inside b187c3a.
  48. in src/wallet/spend.cpp:545 in 62ed8f6a23 outdated
    538@@ -536,31 +539,33 @@ util::Result<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmo
    539     // over all available coins, which would allow mixing.
    540     // If TypesCount() <= 1, there is nothing to mix.
    541     if (allow_mixed_output_types && available_coins.TypesCount() > 1) {
    542-        return ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params);
    543+        const auto& all = available_coins.All();
    544+        Groups groups;
    545+        groups.positive_group = GroupOutputs(wallet, all, coin_selection_params, eligibility_filter, true /* positive_only */);
    546+        groups.mixed_group = GroupOutputs(wallet, all, coin_selection_params, eligibility_filter, false /* positive_only */);
    


    aureleoules commented at 4:04 pm on January 19, 2023:
    nit: same as above (comment before value)

    furszy commented at 1:18 pm on January 20, 2023:
    same answer as above :)
  49. in src/wallet/coinselection.cpp:375 in 5a7ad35fb6 outdated
    371@@ -372,6 +372,42 @@ CAmount OutputGroup::GetSelectionAmount() const
    372     return m_subtract_fee_outputs ? m_value : effective_value;
    373 }
    374 
    375+void OutputGroups::push(const OutputGroup& group, OutputType type, InsertGroupType insert_type)
    


    aureleoules commented at 4:07 pm on January 19, 2023:

    5a7ad35fb682446efb55f93b2f2f07eadd8fc0e3 nit

    0void OutputGroups::Push(const OutputGroup& group, OutputType type, InsertGroupType insert_type)
    

    furszy commented at 10:22 pm on February 5, 2023:
    pushed
  50. in src/wallet/coinselection.cpp:399 in 5a7ad35fb6 outdated
    399+            all_groups.positive_group.emplace_back(group);
    400+            break;
    401+    }
    402+}
    403+
    404+std::optional<Groups> OutputGroups::find(OutputType type)
    


    aureleoules commented at 4:09 pm on January 19, 2023:

    5a7ad35fb682446efb55f93b2f2f07eadd8fc0e3 nit

    0std::optional<Groups> OutputGroups::Find(OutputType type)
    

    furszy commented at 10:22 pm on February 5, 2023:
    pushed
  51. aureleoules commented at 4:25 pm on January 19, 2023: member

    Left some minor comments. Will review 6df2b735b6ad1e744b2ecceeee10fb36a6673fd6, 5a7ad35fb682446efb55f93b2f2f07eadd8fc0e3, and 325f171145771e5ff3a4c7e7eb0966728709370b further later.

    Benchmark results

    ./src/bench/bench_bitcoin -filter='CoinSelection' --min-time=5000.

    Master

    ns/op op/s err% ins/op bra/op miss% total benchmark
    426,716.89 2,343.47 0.3% 10,501,652.22 2,344,328.92 0.2% 5.41 CoinSelection

    PR :rocket:

    ns/op op/s err% ins/op bra/op miss% total benchmark
    328,320.35 3,045.81 0.2% 8,324,339.49 1,946,693.33 0.1% 5.34 CoinSelection

    I ran the benchmarks multiple times to make sure they are consistent.

  52. furszy force-pushed on Jan 20, 2023
  53. furszy force-pushed on Jan 20, 2023
  54. furszy force-pushed on Jan 20, 2023
  55. in src/wallet/test/group_outputs_tests.cpp:176 in 17bd243dfa outdated
    171+    //    "not mine" UTXOs) --> it must not be added to any group
    172+    // ##############################################################################
    173+
    174+    const CTxDestination dest4 = *Assert(wallet->GetNewDestination(OutputType::BECH32, ""));
    175+    addCoin(group_verifier.coins_pool, *wallet, dest4, 6 * COIN,
    176+            /*is_from_me=*/false, CFeeRate(0), /*depth=*/2);
    


    theStack commented at 2:56 pm on January 27, 2023:

    nit: could use the just-below-boundary value of 5 here

    0            /*is_from_me=*/false, CFeeRate(0), /*depth=*/5);
    

    furszy commented at 6:51 pm on January 27, 2023:
    done
  56. in src/wallet/test/group_outputs_tests.cpp:205 in 17bd243dfa outdated
    200+    // ###########################################################################################
    201+    // 7) Surpass the OUTPUT_GROUP_MAX_ENTRIES and verify that a second partial group gets created
    202+    // ###########################################################################################
    203+
    204+    const CTxDestination dest7 = *Assert(wallet->GetNewDestination(OutputType::BECH32, ""));
    205+    for (unsigned long i = 0; i < 130; i++) { // OUTPUT_GROUP_MAX_ENTRIES{100}
    


    theStack commented at 2:57 pm on January 27, 2023:

    nit: could just-above-boundary value of 101 here

    0    for (unsigned long i = 0; i < 101; i++) { // OUTPUT_GROUP_MAX_ENTRIES{100}
    

    furszy commented at 6:51 pm on January 27, 2023:
    done
  57. theStack commented at 2:57 pm on January 27, 2023: contributor
    Concept ACK
  58. furszy force-pushed on Jan 27, 2023
  59. in src/wallet/spend.h:143 in feeae53b0e outdated
    143  *                                                  or (2) an specific error message if there was something particularly wrong (e.g. a selection
    144  *                                                  result that surpassed the tx max weight size).
    145  */
    146-util::Result<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
    147-                        const CoinSelectionParams& coin_selection_params);
    148+util::Result<SelectionResult> ChooseSelectionResult(const CAmount& nTargetValue, Groups groups, const CoinSelectionParams& coin_selection_params);
    


    theStack commented at 4:21 pm on February 4, 2023:

    nit: at this point one could already pass the groups parameter by-reference rather than by-value (it is done later in eca918dd17739036cbb2430a4fc2a5a785d88fb6 though anyways) :

    0util::Result<SelectionResult> ChooseSelectionResult(const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params);
    

    furszy commented at 9:46 pm on February 5, 2023:
    sure, will re-order it so the change appears when Groups are introduced and not later.
  60. in src/wallet/coinselection.h:266 in eca918dd17 outdated
    261+        MIXED_GROUPS, // Insert only to `mixed_group`
    262+        BOTH // Insert to `positive_group` and `mixed_group`
    263+    };
    264+
    265+    // Depending on the 'insert_type', appends the output group into the 'by type' groups
    266+    // and/or the 'all groups' container.
    


    theStack commented at 4:41 pm on February 4, 2023:

    IIUC, for all possible insert_types, something is inserted both in the by type groups and all groups container. The and/or seems to apply for the positive_group/mixed_group? Suggestion:

    0    // Depending on the 'insert_type', appends the output group into the `positive_group`
    1    // and/or the 'mixed_group' for both the `by type` groups and the `all groups` container.
    

    furszy commented at 10:23 pm on February 5, 2023:
    Yeah, thanks!. Aside from the method doc re-phrasing, have re-worked the area, so it’s clear what the push method does on each scenario.
  61. furszy force-pushed on Feb 5, 2023
  62. furszy force-pushed on Feb 5, 2023
  63. furszy force-pushed on Feb 5, 2023
  64. theStack commented at 7:03 pm on February 9, 2023: contributor

    I was thinking if the OutputGroups class could be simplified somehow, right now the methods Push and PushAll seem to be a bit redundant and could be deduplicated. How about a single method like

     0void OutputGroups::Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed)
     1{
     2    if (group.m_outputs.empty()) return;
     3
     4    Groups& groups = groups_by_type[type];	
     5    if (insert_positive && group.GetSelectionAmount() > 0) {
     6        groups.positive_group.emplace_back(group);
     7        all_groups.positive_group.emplace_back(group);
     8    }
     9    if (insert_mixed) {
    10        groups.mixed_group.emplace_back(group);
    11        all_groups.mixed_group.emplace_back(group);
    12    }
    13}
    

    (This seems a bit more like the earlier version you had, but without enums and without duplication.) The methods OutputGroups::PushAll and Groups::GetRef could then simply be dropped and only the call-sites need to be adapted w.r.t. to the two boolean parameters.

  65. furszy force-pushed on Feb 9, 2023
  66. furszy force-pushed on Feb 9, 2023
  67. furszy commented at 10:00 pm on February 9, 2023: member
    Sounds good @theStack 👌🏼, pushed.
  68. furszy force-pushed on Feb 9, 2023
  69. theStack approved
  70. theStack commented at 9:40 pm on February 10, 2023: contributor
    Code-review ACK 0bb90cabc8e2c6bbbc3c52ec1170e725435b625d
  71. achow101 commented at 9:45 pm on February 13, 2023: member
    ACK 0bb90cabc8e2c6bbbc3c52ec1170e725435b625d
  72. furszy requested review from aureleoules on Feb 17, 2023
  73. in src/wallet/spend.cpp:421 in 9c523c5612 outdated
    416@@ -417,13 +417,14 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C
    417             size_t ancestors, descendants;
    418             wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants);
    419 
    420-            // Make an OutputGroup containing just this output
    421-            OutputGroup group{coin_sel_params};
    422-            group.Insert(output, ancestors, descendants, positive_only);
    423+            // If 'positive_only' is set, filter for positive only before adding the coin
    424+            if (!positive_only || (positive_only && output.GetEffectiveValue() > 0)) {
    


    murchandamus commented at 9:11 pm on February 17, 2023:

    Here and everywhere in this commit, the positive_only check is redundant because ¬A ∨ A ∧ B = ¬A ∨ B

    0            if (!positive_only || output.GetEffectiveValue() > 0) {
    

    furszy commented at 9:19 pm on February 22, 2023:
    yeah k 👌🏼, will do the change if need to re-touch to not invalidate the other reviews only for this small redundancy.
  74. S3RK commented at 8:47 am on February 21, 2023: contributor
    Concept ACK. started review
  75. in src/wallet/coinselection.h:251 in 0bb90cabc8 outdated
    247+    void Insert(const std::shared_ptr<COutput>& output, size_t ancestors, size_t descendants);
    248     bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
    249     CAmount GetSelectionAmount() const;
    250 };
    251 
    252+struct Groups {
    


    S3RK commented at 8:28 am on February 22, 2023:

    I believe the separation between positive and mixed group is an implementation detail of particular coin selection algorithms. We can contrast it with CoinEligibilityFilter and OutputType grouping which are algorithm independent. Therefore I propose to not separate positive outputs at the grouping stage.

    Rather filter negative/positive groups as close to the particular coin selection algo as possible, e.g. in ChooseSelectionResult. This will significantly simplify the code by removing extra nesting of structs and multiple if statements. I expect computation overhead of iterating over groups to remove negative UTXos to be negligible, but maybe I’m wrong.


    furszy commented at 8:39 pm on February 22, 2023:

    hmm, that would make the avoid partial spends flow even uglier, plus I don’t think that that will be negligible for big wallets (+50k UTXOs wallets).

    The later removal of negative UTXOs will leave APS full groups with less than 100 entries, which will force us to re-order groups so they are full again (moving UTXOs from one group to another), then if the wallet does not include partial groups, will need to discard those groups too. So making this changes would actually introduce more code than what we currently have with few nested structs (whose creation is scoped only to GroupOutputs). I mean, we would need to have one APS procedure inside GroupOutputs and another reorder + removal APS procedure inside ChooseSelectionResult.

    About the computation overhead: In the worst case scenario, the wallet executes ChooseSelectionResult 5 times in each of the 8 AttemptSelection calls. So, it could end up performing the traverse + re-order + removal 40 times.


    S3RK commented at 9:10 am on February 27, 2023:
    I’m not sure this is how APS works. When we remove negative UTXOs we assess the whole group and not each UTXO individually, so you don’t need to regroup anything after filtering negative groups.

    furszy commented at 7:21 pm on February 27, 2023:

    I’m not sure this is how APS works. When we remove negative UTXOs we assess the whole group and not each UTXO individually, so you don’t need to regroup anything after filtering negative groups.

    No wait, that would leave negative UTXOs in positive-only groups if the sum of the positive UTXOs surpasses the sum of the negative ones.

    Currently, a “positive-only group” is essentially a container who stores only positive UTXOs. Not a mix of UTXOs which values sum is positive. That is different conceptually and something that, at least for now, we are not allowing.

    Check master, inside OutputGroup::Insert method, first line. We reject individual negative UTXOs when the container only stores positive-only UTXOs.

    The only way to create positive-groups out of mixed groups is to traverse each group UTXOs set and remove the negative ones. Then, if the group has less than a 100 entries and there are other groups corresponding to the same script (after doing the same removal on those groups), the wallet need to re-order them so only full groups are provided to the selection algorithms (if the avoid partial groups flag is enabled).

    Aside from that, performance wise, I still think that performing the negative UTXOs removal later on the process will introduce a non-negligible overhead in big wallets.


    S3RK commented at 8:02 pm on February 27, 2023:
    You’re right, thanks for the explanation. This comment could be resolved.

    S3RK commented at 8:32 am on March 2, 2023:
    nit: Judging from the name it’s not clear what’s the difference between Groups and OutputGroups

    furszy commented at 3:47 pm on March 2, 2023:

    maybe we could change Groups to GroupsByEffectiveValue?

    I prefer the struct rather than having an ugly std::pair or a typedef: typedef std::pair<std::vector<OutputGroup>, std::vector<OutputGroup>> GroupsByEffectiveValue


    furszy commented at 10:57 pm on March 2, 2023:
    Ended up renaming OutputGroups to OutputGroupTypeMap to reflect what the struct really does and differentiate it from the inner struct.
  76. murchandamus commented at 9:07 pm on February 22, 2023: contributor
    Concept ACK, very light review, need to go over it in more detail still
  77. in src/wallet/spend.h:112 in a43ee00dd9 outdated
    105@@ -106,7 +106,14 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint&
    106  */
    107 std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
    108 
    109-std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<COutput>& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only);
    110+/**
    111+* Group coins by the provided filters.
    112+*/
    113+OutputGroups GroupOutputs(const CWallet& wallet,
    


    S3RK commented at 7:54 am on March 1, 2023:
    nit: While we are at it, what do you think about passing wallet.chain() instead of whole wallet object?

    furszy commented at 8:32 pm on March 2, 2023:

    yeah could do that, but we wouldn’t win much really. The commit that touches this method signature is the biggest one of the PR, so we would add more diff for no real gain.

    Better to leave it for the next PR on the line. I have few that will come right after this one anyway.

  78. in src/wallet/spend.cpp:481 in a43ee00dd9 outdated
    472+    ScriptPubKeyToOutgroup spk_to_groups_map;
    473+    ScriptPubKeyToOutgroup spk_to_positive_groups_map;
    474+    for (const auto& [type, outs] : coins.coins) {
    475+        for (const COutput& output : outs) {
    476+            // Skip outputs we cannot spend
    477+            if (!output.spendable) continue;
    


    S3RK commented at 7:55 am on March 1, 2023:
    Nit: this is redundant, we shouldn’t have non-spendable coins here at all. They has to be filtered in AvailableCoins

    furszy commented at 3:13 pm on March 2, 2023:
    yes, I didn’t remove it just to make the diff smaller. This was already there.
  79. in src/wallet/coinselection.cpp:375 in a43ee00dd9 outdated
    371@@ -372,6 +372,28 @@ CAmount OutputGroup::GetSelectionAmount() const
    372     return m_subtract_fee_outputs ? m_value : effective_value;
    373 }
    374 
    375+void OutputGroups::Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed)
    


    S3RK commented at 7:59 am on March 1, 2023:
    nit: I’d prefer just having two methods for positive only and for all groups. For me this would be cleaner code instead of controlling the flow with bool params.

    furszy commented at 3:12 pm on March 2, 2023:
    I wrote it in that way before but changed it due a previous code review suggestion (previous diff). I’m not fan of bool args neither but, at least here, I would be ok either way.

    S3RK commented at 9:09 am on March 6, 2023:
    The reason why I suggested this change is because it’s hard to explain why we need a insert_mixed flag. Why don’t we just always add groups to mixed vector? I understand that’s because of how aps works, but just looking at this method it’s mysterious.

    furszy commented at 1:01 pm on March 6, 2023:

    well, this method is just a push function. It doesn’t have to tell anything about the context where it is used. The struct is basically a map container with a cache, it doesn’t need to explain how APS works.

    If you are strong over the bool args usage, that would be the argument to make the changes for me.

  80. in src/wallet/coinselection.h:256 in a43ee00dd9 outdated
    249@@ -249,6 +250,21 @@ struct Groups {
    250     std::vector<OutputGroup> mixed_group;
    251 };
    252 
    253+/** Stores several 'Groups' whose were mapped by output type. */
    254+struct OutputGroups
    255+{
    256+    // Map filters to output groups.
    


    S3RK commented at 8:20 am on March 1, 2023:
    The comment is wrong

    furszy commented at 4:15 pm on March 2, 2023:
    yeah
  81. in src/wallet/coinselection.h:197 in 0bb90cabc8 outdated
    192@@ -193,6 +193,11 @@ struct CoinEligibilityFilter
    193     CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {}
    194     CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {}
    195     CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants), m_include_partial_groups(include_partial) {}
    196+
    197+    bool operator<(const CoinEligibilityFilter& other) const {
    


    S3RK commented at 8:26 am on March 1, 2023:
    Not sure why this is needed

    furszy commented at 3:41 pm on March 2, 2023:
    for the FilteredOutputGroups map. std::map keys requires a comparator or a hash function.
  82. in src/wallet/coinselection.h:255 in 0bb90cabc8 outdated
    251 
    252+struct Groups {
    253+    // Stores 'OutputGroup' containing only positive UTXOs (value > 0).
    254+    std::vector<OutputGroup> positive_group;
    255+    // Stores 'OutputGroup' which may contain both positive and negative UTXOs.
    256+    std::vector<OutputGroup> mixed_group;
    


    S3RK commented at 8:23 am on March 2, 2023:
    nit: it’d be great if we can disambiguate the term mixed. Now it applies to both a) mixed type groups b) mixed effects value group

    furszy commented at 3:20 pm on March 2, 2023:

    What are you referring with “mixed type groups”?

    The mixed term is only used inside the Groups structure, and reflects a vector of OutputGroup that may contain positive and negative UTXOs.


    furszy commented at 8:56 pm on March 2, 2023:
    Ok understood it (talked via DM), the mixed term is also used for the allow_mixed_output_types flag. Which is not from this PR. We could change that boolean to only_select_coins_by_type and forget about the “mixed” term there in a follow-up.
  83. in src/wallet/coinselection.h:264 in 0bb90cabc8 outdated
    260+struct OutputGroups
    261+{
    262+    // Map filters to output groups.
    263+    std::map<OutputType, Groups> groups_by_type;
    264+    // All groups, no filters
    265+    Groups all_groups;
    


    S3RK commented at 8:31 am on March 2, 2023:
    nit: All groups is somewhat misleading here, because these are actually filtered groups, but for all output types. Unfortunately, I don’t have better suggestion yet.

    furszy commented at 3:35 pm on March 2, 2023:

    The “filter” term usage here in the comment is wrong. For what corresponds to this struct, the all term just refers to the combination of all inserted groups. Same as we do in the CoinsResult::All method. The filtering process, nor what is inserted (whether is filtered or not), is not responsibility of this struct.

    It’s just a container that keeps track of Groups by type and the caches the combination of all of them.


    furszy commented at 4:19 pm on March 2, 2023:

    I mean, it would be slower but could also be created on-demand:

    0Groups OutputGroups::All()
    1{
    2    Groups ret;
    3    for (const auto& [type, groups_vec] : groups_by_type) {
    4        ret.positive_group.insert(ret.positive_group.begin(), groups_vec.positive_group.begin(), groups_vec.positive_group.end());
    5        ret.mixed_group.insert(ret.mixed_group.begin(), groups_vec.mixed_group.begin(), groups_vec.mixed_group.end());
    6    }
    7    return ret;
    8}
    

    furszy commented at 10:57 pm on March 2, 2023:
    Fixed the “filter” comment mention.
  84. S3RK commented at 8:35 am on March 2, 2023: contributor

    Code review ACK 0bb90cabc8e2c6bbbc3c52ec1170e725435b625d modulo tests.

    I believe this PR is a good improvement over status quo.

    The biggest worry I have now it somewhat ambiguous terminology:

    • mixed groups could mean mixed type or mixed effective value
    • all groups not always refers to actually all groups, but filtered by CoinEligibiltyFilter
    • two structs Groups vs OutputGroups

    I don’t have better suggestion as of now, so it’s not blocking. I’ll think more and come back if I have some ideas. Maybe @Xekyo the master of terminology have some ideas.

  85. furszy force-pushed on Mar 2, 2023
  86. furszy commented at 10:56 pm on March 2, 2023: member

    Thanks @S3RK for the review.

    Updated per feedback (small diff), tackled the following points that made sense for me to include here:

    • Renamed OutputGroups to OutputGroupTypeMap to reflect what the struct really does and differentiate it from the inner struct.
    • Corrected the OutputGroups, now OutputGroupTypeMap, members comments.
    • And tackled @Xekyo’s unneeded check.

    I think that while we all agree that this work is functionality and structurally sound, we can move forward. And tackle any new naming discussion in a follow-up work.

  87. wallet: make OutputGroup "positive_only" filter explicit
    And not hide it inside the `OutputGroup::Insert` method.
    This method does not return anything if insertion fails.
    
    We can know before calling `Insert` whether the coin
    will be accepted or not.
    06ec8f9928
  88. furszy force-pushed on Mar 3, 2023
  89. furszy commented at 9:36 pm on March 3, 2023: member
    Rebased due silent merge conflicts with #26889. Ready to go now.
  90. in src/wallet/test/group_outputs_tests.cpp:210 in 7f39f0612d outdated
    205+    uint16_t NUM_SINGLE_ENTRIES = 101;
    206+    for (unsigned long i = 0; i < NUM_SINGLE_ENTRIES; i++) { // OUTPUT_GROUP_MAX_ENTRIES{100}
    207+        addCoin(group_verifier.coins_pool, *wallet, dest7, 9 * COIN, /*is_from_me=*/true);
    208+    }
    209+
    210+    // Exclude partial groups must have no changes from this round and the previous one (point 6)
    


    S3RK commented at 8:38 am on March 6, 2023:

    this is slightly confusing because I’d expect the number to go one up from the previous case (from 3 => 4).

    The test is passing, because you flipped positive_only and now excluded negative group is balanced by the group for dest7.

    I’d suggest keeping positive_only=false and bumping the numbers


    furszy commented at 1:02 pm on March 6, 2023:
    sounds good, pushed
  91. in src/wallet/test/group_outputs_tests.cpp:207 in 7f39f0612d outdated
    196+            /*expected_with_partial_spends_size=*/ GROUP_SIZE * 2 + 1,
    197+            /*expected_without_partial_spends_size=*/ 3,
    198+            /*positive_only=*/ false);
    199+
    200+    // ###########################################################################################
    201+    // 7) Surpass the OUTPUT_GROUP_MAX_ENTRIES and verify that a second partial group gets created
    


    S3RK commented at 8:48 am on March 6, 2023:
    Commit message is now inconsistent with the code. Either drop the details of test cases or update them.

    furszy commented at 1:03 pm on March 6, 2023:
    pushed, thanks
  92. S3RK commented at 9:08 am on March 6, 2023: contributor
    utACK 1cfed6ce6cd8c9f9f1e93662ff8f92cd5a5b91c4 (also reviewed the tests)
  93. test: wallet, add coverage for outputs grouping process
    The following scenarios are covered:
    
    1) 10 UTXO with the same script:
       partial spends is enabled --> outputs must not be grouped.
    
    2) 10 UTXO with the same script:
       partial spends disabled --> outputs must be grouped.
    
    3) 20 UTXO, 10 one from scriptA + 10 from scriptB:
       a) if partial spends is enabled --> outputs must not be grouped.
       b) if partial spends is not enabled --> 2 output groups expected (one per script).
    
    3) Try to add a negative output (value - fee < 0):
       a) if "positive_only" is enabled --> negative output must be skipped.
       b) if "positive_only" is disabled --> negative output must be added.
    
    4) Try to add a non-eligible UTXO (due not fulfilling the min depth target for
     "not mine" UTXOs) --> it must not be added to any group
    
    5) Try to add a non-eligible UTXO (due not fulfilling the min depth target for
     "mine" UTXOs) --> it must not be added to any group
    
    6) Surpass the 'OUTPUT_GROUP_MAX_ENTRIES' size and verify that a second partial
    group gets created.
    d8e749bb84
  94. refactor: make OutputGroup::m_outputs field a vector of shared_ptr
    Initial steps towards sharing COutput instances across all possible
    OutputGroups (instead of copying them time after time).
    461f0821a2
  95. wallet: decouple outputs grouping process from each ChooseSelectionResult
    Another step towards the single OutputGroups calculation goal
    34f54a0a3a
  96. test: coinselector_tests refactor, use CoinsResult instead of plain std::vector
    No functional changes. Only cosmetic changes to simplify the follow-up commit.
    55962001da
  97. wallet: unify outputs grouping process
    The 'GroupOutputs()' function performs the same
    calculations for only-positive and mixed groups,
    the only difference is that when we look for
    only-positive groups, we discard negative utxos.
    
    So, instead of wasting resources calling GroupOutputs()
    for positive-only first, then call it again to include
    the negative ones in the result, we can execute
    GroupOutputs() only once, including in the response
    both group types (positive-only and mixed).
    bd91ed1cb2
  98. wallet: single output groups filtering and grouping process
    Optimizes coin selection by performing the "group outputs"
    procedure only once, outside the "attempt selection" process.
    
    Avoiding the repeated execution of the 'GroupOutputs' operation
    that occurs on each coin eligibility filters (up to 8 of them);
    then for every coin vector type plus one for all the coins together.
    
    This also let us not perform coin selection over coin eligibility
    filtered groups that don't add new elements.
    (because, if the previous round failed, and the subsequent one has
    the same coins, then this new round will fail again).
    6a302d40df
  99. DrahtBot requested review from achow101 on Mar 6, 2023
  100. DrahtBot requested review from theStack on Mar 6, 2023
  101. furszy force-pushed on Mar 6, 2023
  102. furszy commented at 1:06 pm on March 6, 2023: member

    Updated per feedback, thanks S3RK 👌🏼.

    Small diff, changes are in the last test scenario and the test commit message.

  103. S3RK commented at 7:43 pm on March 6, 2023: contributor
    ReACK 6a302d4 Thanks for addressing feedback
  104. theStack approved
  105. theStack commented at 9:31 pm on March 6, 2023: contributor

    re-ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44 :coconut:

    (verified via git range-diff 0bb90cab...6a302d40 that since my last ACK, most review comments by Murch and S3RK have been tackled, mostly trivial refactor + terminology/naming improvements and a minor unit test adaption)

  106. achow101 commented at 11:19 pm on March 6, 2023: member
    ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44
  107. achow101 merged this on Mar 6, 2023
  108. achow101 closed this on Mar 6, 2023

  109. sidhujag referenced this in commit 62f50a7371 on Mar 7, 2023
  110. achow101 referenced this in commit 609c95d4a8 on Mar 15, 2023
  111. sidhujag referenced this in commit dd7f9c4f68 on Mar 16, 2023
  112. sidhujag referenced this in commit f979be15ac on Mar 16, 2023
  113. in src/wallet/test/fuzz/coinselection.cpp:32 in 06ec8f9928 outdated
    28@@ -29,8 +29,10 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect
    29     auto output_group = OutputGroup(coin_params);
    30     bool valid_outputgroup{false};
    31     for (auto& coin : coins) {
    32-        output_group.Insert(coin, /*ancestors=*/0, /*descendants=*/0, positive_only);
    33-        // If positive_only was specified, nothing may have been inserted, leading to an empty output group
    34+        if (!positive_only || (positive_only && coin.GetEffectiveValue() > 0)) {
    


    murchandamus commented at 4:54 pm on April 10, 2023:
    0        if (!positive_only || (coin.GetEffectiveValue() > 0)) {
    
  114. murchandamus commented at 5:14 pm on April 10, 2023: contributor
    Post merge ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44
  115. furszy deleted the branch on May 27, 2023
  116. bitcoin locked this on May 26, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me