wallet: add tracepoints and algorithm information to coin selection #24644

pull achow101 wants to merge 5 commits into bitcoin:master from achow101:tracepoints-coin-selection changing 7 files +297 −10
  1. achow101 commented at 9:28 pm on March 22, 2022: member

    Tracepoints can be useful for coin selection as they would allow us to observe what is being selected, selection parameters, and calculation results. So this PR adds 4 new tracepoints:

    1. After SelectCoins returns in order to observe the SelectionResult
    2. After the first CreateTransactionInternal to observe the created transaction
    3. Prior to the second CreateTransactionInternal to notify that the optimistic avoid partial spends selection is occurring
    4. After the second CreateTransactionInternal to observe the created transaction and inform which solution is being used.

    This PR also adds an algorithm enum to SelectionResult so that the first tracepoint will be able to report which algorithm was used to produce that result.

    The primary use case for these tracepoints is in running coin selection simulations. The script I use to run these simulations use these tracepoints in order to gather data on the algorithm used and the calculated waste.

  2. DrahtBot added the label Wallet on Mar 22, 2022
  3. josibake commented at 9:44 pm on March 22, 2022: member
    Concept ACK
  4. S3RK commented at 8:39 am on March 23, 2022: member

    Huge Concept ACK!

    Looking at existing tracing contexts, it seems wallet context would be better level of granularity. Also doc/tracing.md needs to be updated to mention new context.

    Could you also add an example to contrib/tracing about how to use the new tracing points?

  5. in src/wallet/coinselection.h:249 in 4fed26af96 outdated 
    205+    BNB = 0,
206+    KNAPSACK = 1,
207+    SRD = 2,
208+    MANUAL = 3,
209+};
210+
    vincenzopalazzo commented at 9:09 am on March 23, 2022:

    There is any motivation to use the plain enum here and not the enum class?

    Maybe I’m missing some things but usually, the enum class is preferred in C++.

    achow101 commented at 5:18 pm on March 23, 2022:
    Done
  6. fanquake commented at 9:47 am on March 23, 2022: member
    cc @0xB10C
  7. 0xB10C commented at 10:07 am on March 23, 2022: member

    Cool, Concept ACK! Will review.

    We might want to start adding tracepoint interface tests (see #24358) in the same PR when adding new tracepoints. This makes review a lot easier and provides an usage example.

  8. achow101 force-pushed on Mar 23, 2022
  9. vincenzopalazzo approved
  11. DrahtBot commented at 4:13 pm on March 24, 2022: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24935 (mempool: Add usdt event tracepoints and eBPF logging tool. by russeree)
    • #24897 ([Draft / POC] Silent Payments by w0xlt)
    • #24845 (wallet: createTransaction, return proper error description for “too-long-mempool-chain” + introduce generic Result classes by furszy)
    • #24752 (wallet: increase BnB upper limit by S3RK)
    • #20640 (wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack)

    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.

  12. DrahtBot added the label Needs rebase on Mar 24, 2022
  13. achow101 force-pushed on Mar 25, 2022
  14. DrahtBot removed the label Needs rebase on Mar 25, 2022
  15. DrahtBot added the label Needs rebase on Mar 25, 2022
  16. achow101 force-pushed on Mar 26, 2022
  17. jb55 commented at 0:29 am on March 26, 2022: member
    Concept ACK
  18. DrahtBot removed the label Needs rebase on Mar 26, 2022
  19. achow101 force-pushed on Mar 30, 2022
  20. jonatack commented at 12:41 pm on April 4, 2022: member
    Concept ACK
  21. 0xB10C commented at 9:05 am on April 14, 2022: member

    fwiw: I think to move this forward at least documentation in doc/tracing.md#tracepoint-documentation and interface tests similar to #24358 are needed.

    I’m not too familiar with coin selection and don’t know what e.g. the coin_selection:aps_create_tx_internal tracepoint traces. This makes reviewing cumbersome for me.

  22. wallet: track which coin selection algorithm produced a SelectionResult 912f1ed181
  23. wallet: compute waste for SelectionResults of preset inputs 
    When we use only manually specified inputs, we should still calculate
the waste so that if anything later on calls GetWaste (in order to log
it), there won't be an error.
    15b58383d0
  24. wallet: Add some tracepoints for coin selection 8e3f39e4fa
  25. achow101 commented at 6:38 pm on April 14, 2022: member
    I’ve added docs and a test.
  26. achow101 force-pushed on Apr 14, 2022
  27. laanwj commented at 6:59 pm on April 14, 2022: member
    Concept ACK, thanks for adding tracepoints. I’ll try to have a more detailed look at this soon.
  28. in doc/tracing.md:221 in e1aff5c2db outdated 
    216+Is called when `CreateTransactionInternal` is called the second time for the optimistic
217+Avoid Partial Spends selection attempt. This is used to determine whether the next
218+tracepoints called are for the Avoid Partial Spends solution, or a different transaction.
219+
220+Arguments passed:
221+1. Wallet name as `pointer to C-style string`
    0xB10C commented at 8:57 am on April 21, 2022:
    duplicate docs for coin_selection:attempting_aps_create_tx
    achow101 commented at 3:17 pm on April 21, 2022:
    Oops, removed.
  29. in test/functional/interface_usdt_coinselection.py:11 in 421c931976 outdated 
     6+"""  Tests the coin_selection:* tracepoint API interface.
 7+     See https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#context-coin_selection
 8+"""
 9+
10+import ctypes
11+from io import BytesIO
    0xB10C commented at 9:04 am on April 21, 2022:

    CI linter complains:

    0test/functional/interface_usdt_coinselection.py:10:1: F401 'ctypes' imported but unused
1test/functional/interface_usdt_coinselection.py:11:1: F401 'io.BytesIO' imported but unused
    achow101 commented at 3:17 pm on April 21, 2022:
    Removed
  30. in test/functional/interface_usdt_coinselection.py:125 in 421c931976 outdated 
    122+                # If the loop exits successfully instead of throwing a KeyError, then we have had
123+                # more events than expected. There should be no more than 5 events.
124+                assert False
125+        except KeyError:
126+            assert_equal(len(events), len(expected_types))
127+            return events
    0xB10C commented at 9:15 am on April 21, 2022:

    I found this to be unclear, probably because I was unfamiliar with Python’s for/else. If I understand correctly, you want to check that exactly len(expected_types) tracepoints were triggered?

     0        try:
 1            for i in range(0, len(expected_types) + 1):
 2                event = self.bpf["coin_selection_events"].pop()
 3                assert_equal(event.wallet_name.decode(), self.default_wallet_name)
 4                assert_equal(event.type, expected_types[i])
 5                events.append(event)
 6            else:
 7                # If the loop exits successfully instead of throwing a KeyError, then we have had
 8                # more events than expected. There should be no more than `len(expected_types)` events.
 9                assert False
10        except KeyError:
11            assert_equal(len(events), len(expected_types))
12            return events
    achow101 commented at 3:18 pm on April 21, 2022:
    Done
  31. in test/functional/interface_usdt_coinselection.py:42 in 421c931976 outdated 
    37+    s64 target;
38+    s64 waste;
39+    s64 selected_value;
40+
41+    // create tx event
42+    u8 success;
    0xB10C commented at 9:19 am on April 21, 2022: 
    0    bool success;
    achow101 commented at 3:18 pm on April 21, 2022:
    Done
  32. in test/functional/interface_usdt_coinselection.py:47 in 421c931976 outdated 
    42+    u8 success;
43+    s64 fee;
44+    s32 change_pos;
45+
46+    // aps create tx event
47+    u8 use_aps;
    0xB10C commented at 9:19 am on April 21, 2022: 
    0    bool use_aps;
    achow101 commented at 3:18 pm on April 21, 2022:
    Done
  33. in test/functional/interface_usdt_coinselection.py:146 in 421c931976 outdated 
    141+                if not is_aps:
142+                    algo = event.algo.decode()
143+                    waste = event.waste
144+                sc_events.append(event)
145+            elif event.type == 2:
146+                success = event.success == 1
    0xB10C commented at 9:20 am on April 21, 2022:

    with event.success being a bool rather than a u8

    0                success = event.success
    achow101 commented at 3:18 pm on April 21, 2022:
    Done
  34. in test/functional/interface_usdt_coinselection.py:153 in 421c931976 outdated 
    148+                    change_pos = event.change_pos
149+            elif event.type == 3:
150+                is_aps = True
151+            elif event.type == 4:
152+                assert is_aps
153+                if event.use_aps == 1:
    0xB10C commented at 9:21 am on April 21, 2022:

    with event.use_aps being a bool rather than a u8

    0                if event.use_aps:
    achow101 commented at 3:18 pm on April 21, 2022:
    Done
  35. in test/functional/interface_usdt_coinselection.py:183 in 421c931976 outdated 
    180+        # 5. aps_create_tx_internal (type 4)
181+        wallet.sendtoaddress(wallet.getnewaddress(), 10)
182+        events = self.get_tracepoints([1, 2, 3, 1, 4])
183+        success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events)
184+        assert_equal(success, True)
185+        assert_greater_than(change_pos, -1)
    0xB10C commented at 9:26 am on April 21, 2022:
    does it make sense to check use_aps, algo, and waste here too?
    achow101 commented at 3:19 pm on April 21, 2022:
    No, this test doesn’t make any assumptions about what coin selection algorithm was used.
    glozow commented at 10:17 pm on April 22, 2022:
    As a sanity check that it’s not malformed, you could assert algo in ["bnb", "srd", "knapsack", "manual"]
  36. in test/functional/interface_usdt_coinselection.py:202 in 421c931976 outdated 
    199+        wallet.setwalletflag("avoid_reuse")
200+        wallet.sendtoaddress(address=wallet.getnewaddress(), amount=10, avoid_reuse=True)
201+        events = self.get_tracepoints([1, 2])
202+        success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events)
203+        assert_equal(success, True)
204+        assert_equal(use_aps, None)
    0xB10C commented at 9:29 am on April 21, 2022:
    does it make sense to check algo, change_pos, and waste here too?
    achow101 commented at 3:19 pm on April 21, 2022:
    No, this test doesn’t make any assumptions about what coin selection algorithm was used.
  37. 0xB10C commented at 9:55 am on April 21, 2022: member

    The tracepoint part looks good. Left a few comments.

    I also wrote a quick and dirty bpftrace script to test the documentation. In case that’s helpful to anyone:

     0#!/usr/bin/env bpftrace
 1
 2BEGIN
 3{
 4  printf("Logging coin selection\n")
 5}
 6
 7usdt:./src/bitcoind:coin_selection:selected_coins
 8{
 9  $name = str(arg0);
10  $algo = str(arg1);
11  $target = (int64) arg2;
12  $waste = (int64) arg3;
13  $inputsum = (int64) arg4;
14  printf("selected_coins: wallet=%s, algo=%s, target=%ld, waste=%ld, inputsum=%ld\n", $name, $algo, $target, $waste, $inputsum);
15}
16
17usdt:./src/bitcoind:coin_selection:normal_create_tx_internal
18{
19  $name = str(arg0);
20  $success = arg1;
21  $fee = (int64) arg2;
22  $change_pos = (int32) arg3;
23  printf("normal_create_tx_internal: wallet=%s, success=%d, fee=%d, change_pos=%d\n", $name, $success, $fee, $change_pos);
24}
25
26usdt:./src/bitcoind:coin_selection:attempting_aps_create_tx
27{
28  $name = str(arg0);
29  printf("attempting_aps_create_tx: wallet=%s\n", $name);
30}
31
32usdt:./src/bitcoind:coin_selection:aps_create_tx_internal
33{
34  $name = str(arg0);
35  $use_aps = arg1;
36  $success = arg2;
37  $fee = (int64) arg3;
38  $change_pos = (int32) arg4;
39  printf("aps_create_tx_internal: wallet=%s, use_aps=%d, success=%d, fee=%ld, change_pos=%d\n", $name, $use_aps, $success, $fee, $change_pos);
40}

    Running e.g. the wallet_taproot.py functional test logs negative values for some fields (target, inputsum) where I didn’t expect them. I’m not sure what’s up with that.

    0selected_coins: wallet=rpc_online, algo=bnb, target=-1436883448, waste=118372, inputsum=-1436883448
1aps_create_tx_internal: wallet=rpc_online, use_aps=1, success=1, fee=131993, change_pos=-1
2selected_coins: wallet=psbt_online, algo=bnb, target=-2146144343, waste=95374, inputsum=-2146144343
3normal_create_tx_internal: wallet=psbt_online, success=1, fee=107994, change_pos=-1
4attempting_aps_create_tx: wallet=psbt_online
5selected_coins: wallet=psbt_online, algo=bnb, target=-2146144343, waste=95374, inputsum=-2146144343
6aps_create_tx_internal: wallet=psbt_online, use_aps=1, success=1, fee=107994, change_pos=-1
  38. doc: document coin selection tracepoints ca02b68e8a
  39. test: Add test for coinselection tracepoints ab5af9ca72
  40. achow101 force-pushed on Apr 21, 2022
  41. achow101 commented at 3:56 pm on April 21, 2022: member

    Running e.g. the wallet_taproot.py functional test logs negative values for some fields (target, inputsum) where I didn’t expect them. I’m not sure what’s up with that.

    Your script uses %d instead of %ld. %d is for 32-bit ints, whereas %ld is for 64 bit.

  42. jb55 commented at 4:18 pm on April 21, 2022: member
    crACK ab5af9ca7293239ffc24ea7e23159b8184543f94
  43. 0xB10C approved
  44. 0xB10C commented at 4:34 pm on April 21, 2022: member
    ACK ab5af9ca7293239ffc24ea7e23159b8184543f94. Code reviewed, ran the interface_usdt_coinselection.py test, and tested with the above bpftrace script (updated %d -> %ld where necessary, ty achow101).
  45. in src/wallet/coinselection.h:264 in 8e3f39e4fa outdated 
    259     /** Whether the input values for calculations should be the effective value (true) or normal value (false) */
260     bool m_use_effective{false};
261     /** The computed waste */
262     std::optional<CAmount> m_waste;
263-    /** The algorithm used to produce this result */
264-    SelectionAlgorithm m_algo;
    josibake commented at 12:10 pm on April 26, 2022:
    why is m_algo added as private and then moved here to public? wouldn’t it be better to move m_target to public and add m_algo in the first commit?
    achow101 commented at 2:04 pm on April 26, 2022:
    Indeed, I forgot to squash those separately when implementing this. Will move if I have to re-touch.
  46. josibake approved
  47. josibake commented at 12:11 pm on April 26, 2022: member

    crACK https://github.com/bitcoin/bitcoin/pull/24644/commits/ab5af9ca7293239ffc24ea7e23159b8184543f94

    looks good! just had one nit about commit structure. also compiled and ran the functional test without any issues

  48. fanquake requested review from laanwj on Apr 26, 2022
  49. josibake commented at 6:00 pm on April 26, 2022: member

    tACK https://github.com/bitcoin/bitcoin/commit/ab5af9ca7293239ffc24ea7e23159b8184543f94

    wasn’t paying attention earlier and it was actually skipping the USDT functional tests, not running them :facepalm: .

    I got tracepoints working by running sudo apt install systemtap-sdt-dev (per doc/build-unix.md) and building bcc from source (https://github.com/iovisor/bcc/blob/master/INSTALL.md#ubuntu---source)

    everything works as expected. i also noticed test/functional/wallet_send.py is failing in the CI, but i the test locally with no issues and then re-ran the whole test suite, so perhaps just a transient failure due to a timeout?

  50. fanquake merged this on Apr 26, 2022
  51. fanquake closed this on Apr 26, 2022
  52. w0xlt commented at 8:20 pm on April 27, 2022: contributor
    post-merged tACK https://github.com/bitcoin/bitcoin/pull/24644/commits/ab5af9ca7293239ffc24ea7e23159b8184543f94 Functional test ran successfully on Ubuntu 21.10 and bcc (master version).
  53. 0xB10C commented at 9:31 am on April 28, 2022: member
    ref: #25003 tracing: fix coin_selection:aps_create_tx_internal calling logic
  54. russeree commented at 8:32 am on April 30, 2022: contributor

    Tests will fail on RHEL linux when default yum dist. of bcc is used. To remedy bcc needs to be build from bcc git master.

    Error output https://gist.github.com/russeree/39a3f13aababe44ba8bac52224626d0b

    After building and running the current master of bcc https://gist.github.com/russeree/888c52a85e9bcd69c758ff0bc91f3d13

    This issue only presents on RHEL (Fedora, Centos, Redhat) Ubuntu seemed to not be affected by this using apt installed versions of bcc.

  55. DrahtBot locked this on Apr 30, 2023


achow101 josibake S3RK vincenzopalazzo fanquake 0xB10C DrahtBot jb55 jonatack laanwj glozow w0xlt russeree


laanwj

Labels
Wallet

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-11-17 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me