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

achow101 · 2022-03-22T21:28:03Z

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:

After SelectCoins returns in order to observe the SelectionResult
After the first CreateTransactionInternal to observe the created transaction
Prior to the second CreateTransactionInternal to notify that the optimistic avoid partial spends selection is occurring
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.

josibake · 2022-03-22T21:44:49Z

Concept ACK

S3RK · 2022-03-23T08:39:50Z

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?

src/wallet/coinselection.h

fanquake · 2022-03-23T09:47:43Z

cc @0xB10C

0xB10C · 2022-03-23T10:07:33Z

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.

vincenzopalazzo

ACK 64338c1

DrahtBot · 2022-03-24T16:13:03Z

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.

jb55 · 2022-03-26T00:29:16Z

Concept ACK

jonatack · 2022-04-04T12:41:45Z

Concept ACK

0xB10C · 2022-04-14T09:05:21Z

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.

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.

achow101 · 2022-04-14T18:38:02Z

I've added docs and a test.

laanwj · 2022-04-14T18:59:07Z

Concept ACK, thanks for adding tracepoints. I'll try to have a more detailed look at this soon.

0xB10C · 2022-04-21T09:55:02Z

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:

bpftrace script

#!/usr/bin/env bpftrace

BEGIN
{
  printf("Logging coin selection\n")
}

usdt:./src/bitcoind:coin_selection:selected_coins
{
  $name = str(arg0);
  $algo = str(arg1);
  $target = (int64) arg2;
  $waste = (int64) arg3;
  $inputsum = (int64) arg4;
  printf("selected_coins: wallet=%s, algo=%s, target=%ld, waste=%ld, inputsum=%ld\n", $name, $algo, $target, $waste, $inputsum);
}

usdt:./src/bitcoind:coin_selection:normal_create_tx_internal
{
  $name = str(arg0);
  $success = arg1;
  $fee = (int64) arg2;
  $change_pos = (int32) arg3;
  printf("normal_create_tx_internal: wallet=%s, success=%d, fee=%d, change_pos=%d\n", $name, $success, $fee, $change_pos);
}

usdt:./src/bitcoind:coin_selection:attempting_aps_create_tx
{
  $name = str(arg0);
  printf("attempting_aps_create_tx: wallet=%s\n", $name);
}

usdt:./src/bitcoind:coin_selection:aps_create_tx_internal
{
  $name = str(arg0);
  $use_aps = arg1;
  $success = arg2;
  $fee = (int64) arg3;
  $change_pos = (int32) arg4;
  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);
}

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.

selected_coins: wallet=rpc_online, algo=bnb, target=-1436883448, waste=118372, inputsum=-1436883448
aps_create_tx_internal: wallet=rpc_online, use_aps=1, success=1, fee=131993, change_pos=-1
selected_coins: wallet=psbt_online, algo=bnb, target=-2146144343, waste=95374, inputsum=-2146144343
normal_create_tx_internal: wallet=psbt_online, success=1, fee=107994, change_pos=-1
attempting_aps_create_tx: wallet=psbt_online
selected_coins: wallet=psbt_online, algo=bnb, target=-2146144343, waste=95374, inputsum=-2146144343
aps_create_tx_internal: wallet=psbt_online, use_aps=1, success=1, fee=107994, change_pos=-1

doc/tracing.md

test/functional/interface_usdt_coinselection.py

0xB10C · 2022-04-21T09:26:50Z

test/functional/interface_usdt_coinselection.py

+        assert_equal(success, True)
+        assert_greater_than(change_pos, -1)


does it make sense to check use_aps, algo, and waste here too?

No, this test doesn't make any assumptions about what coin selection algorithm was used.

As a sanity check that it's not malformed, you could assert algo in ["bnb", "srd", "knapsack", "manual"]

0xB10C · 2022-04-21T09:29:04Z

test/functional/interface_usdt_coinselection.py

+        events = self.get_tracepoints([1, 2])
+        success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events)
+        assert_equal(success, True)
+        assert_equal(use_aps, None)


does it make sense to check algo, change_pos, and waste here too?

No, this test doesn't make any assumptions about what coin selection algorithm was used.

achow101 · 2022-04-21T15:56:36Z

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.

jb55 · 2022-04-21T16:18:35Z

crACK ab5af9c

0xB10C

ACK ab5af9c. Code reviewed, ran the interface_usdt_coinselection.py test, and tested with the above bpftrace script (updated %d -> %ld where necessary, ty achow101).

josibake

crACK ab5af9c

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

josibake · 2022-04-26T12:10:28Z

src/wallet/coinselection.h

-    /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */
-    const CAmount m_target;
    /** Whether the input values for calculations should be the effective value (true) or normal value (false) */
    bool m_use_effective{false};
    /** The computed waste */
    std::optional<CAmount> m_waste;
-    /** The algorithm used to produce this result */
-    SelectionAlgorithm m_algo;


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?

Indeed, I forgot to squash those separately when implementing this. Will move if I have to re-touch.

josibake · 2022-04-26T18:00:53Z

tACK ab5af9c

wasn't paying attention earlier and it was actually skipping the USDT functional tests, not running them 🤦 .

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?

w0xlt

post-merged tACK ab5af9c
Functional test ran successfully on Ubuntu 21.10 and bcc (master version).

0xB10C · 2022-04-28T09:31:54Z

ref: #25003 tracing: fix coin_selection:aps_create_tx_internal calling logic

portlandhodl · 2022-04-30T08:32:55Z

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.

DrahtBot added the Wallet label Mar 22, 2022

vincenzopalazzo reviewed Mar 23, 2022

View reviewed changes

src/wallet/coinselection.h Outdated Show resolved Hide resolved

achow101 force-pushed the tracepoints-coin-selection branch from 4fed26a to 64338c1 Compare March 23, 2022 17:18

vincenzopalazzo approved these changes Mar 24, 2022

View reviewed changes

DrahtBot mentioned this pull request Mar 24, 2022

wallet: generate random change target for each tx for better privacy #24494

Merged

DrahtBot added the Needs rebase label Mar 24, 2022

DrahtBot mentioned this pull request Mar 25, 2022

wallet: Consolidate CInputCoin and COutput #24091

Merged

achow101 force-pushed the tracepoints-coin-selection branch from 64338c1 to 1a836be Compare March 25, 2022 18:27

DrahtBot added Needs rebase and removed Needs rebase labels Mar 25, 2022

achow101 force-pushed the tracepoints-coin-selection branch from 1a836be to 27cf3c6 Compare March 26, 2022 00:16

DrahtBot removed the Needs rebase label Mar 26, 2022

achow101 force-pushed the tracepoints-coin-selection branch from 27cf3c6 to 24e1d7c Compare March 30, 2022 18:34

DrahtBot mentioned this pull request Apr 2, 2022

wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

Merged

DrahtBot mentioned this pull request Apr 5, 2022

wallet: increase BnB upper limit #24752

Closed

DrahtBot mentioned this pull request Apr 14, 2022

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

Merged

achow101 added 3 commits April 14, 2022 12:40

wallet: track which coin selection algorithm produced a SelectionResult

912f1ed

wallet: compute waste for SelectionResults of preset inputs

15b5838

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.

wallet: Add some tracepoints for coin selection

8e3f39e

achow101 force-pushed the tracepoints-coin-selection branch from 24e1d7c to 421c931 Compare April 14, 2022 18:38

DrahtBot mentioned this pull request Apr 17, 2022

[Draft / POC] Silent Payments #24897

Closed

0xB10C reviewed Apr 21, 2022

View reviewed changes

DrahtBot mentioned this pull request Apr 21, 2022

mempool: Add usdt event tracepoints and eBPF logging tool. #24935

Closed

achow101 added 2 commits April 21, 2022 11:11

doc: document coin selection tracepoints

ca02b68

test: Add test for coinselection tracepoints

ab5af9c

achow101 force-pushed the tracepoints-coin-selection branch from 421c931 to ab5af9c Compare April 21, 2022 15:17

0xB10C approved these changes Apr 21, 2022

View reviewed changes

josibake approved these changes Apr 26, 2022

View reviewed changes

fanquake requested a review from laanwj April 26, 2022 15:53

fanquake merged commit 260ede1 into bitcoin:master Apr 26, 2022

w0xlt reviewed Apr 27, 2022

View reviewed changes

murchandamus mentioned this pull request May 10, 2022

tracing: fix coin_selection:aps_create_tx_internal calling logic #25003

Merged

bitcoin locked and limited conversation to collaborators Apr 30, 2023

		assert_equal(success, True)
		assert_greater_than(change_pos, -1)

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

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

Uh oh!

Conversation

achow101 commented Mar 22, 2022

Uh oh!

josibake commented Mar 22, 2022

Uh oh!

S3RK commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fanquake commented Mar 23, 2022

Uh oh!

0xB10C commented Mar 23, 2022

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jb55 commented Mar 26, 2022

Uh oh!

jonatack commented Apr 4, 2022

Uh oh!

0xB10C commented Apr 14, 2022

Uh oh!

achow101 commented Apr 14, 2022

Uh oh!

laanwj commented Apr 14, 2022

Uh oh!

0xB10C commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xB10C Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

glozow Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

0xB10C Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 commented Apr 21, 2022

Uh oh!

jb55 commented Apr 21, 2022 via email

Uh oh!

0xB10C left a comment

Choose a reason for hiding this comment

Uh oh!

josibake left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

josibake commented Apr 26, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

S3RK commented Mar 23, 2022 •

edited

Loading

DrahtBot commented Mar 24, 2022 •

edited

Loading

0xB10C commented Apr 21, 2022 •

edited

Loading

josibake left a comment •

edited

Loading

josibake Apr 26, 2022 •

edited

Loading