doc, test: more ephemeral dust follow-ups

theStack commented at 8:18 pm on November 25, 2024: contributor

This small PR contains ephemeral dust follow-ups mentioned in #30329 that were not tackled in the first follow-up PR #31279:

#30239 (review) #30239 (review)

Happy to add more if I missed some or anyone has concrete commits to add.

doc: ephemeral policy: add missing closing double quote 61e18dec30

DrahtBot commented at 8:18 pm on November 25, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31371.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, instagibbs, tdb3

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:

#31356 (Add and use satToBtc and btcToSat util functions by andremralves)

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.

test: refactor: introduce `create_ephemeral_dust_package` helper 160799d913

theStack force-pushed on Nov 25, 2024

DrahtBot added the label CI failed on Nov 25, 2024

DrahtBot removed the label CI failed on Nov 25, 2024

in test/functional/mempool_ephemeral_dust.py:54 in 160799d913

46@@ -47,6 +47,22 @@ def add_output_to_create_multi_result(self, result, output_value=0):
47 
48         result["new_utxos"].append({"txid": new_txid, "vout": len(result["tx"].vout) - 1, "value": Decimal(output_value) / COIN, "height": 0, "coinbase": False, "confirmations": 0})
49 
50+    def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsors=None):
51+        """Creates a 1P1C package containing ephemeral dust. By default, the parent transaction
52+           is zero-fee and creates a single zero-value dust output, and all of its outputs are
53+           spent by the child."""
54+        dusty_tx = self.wallet.create_self_transfer_multi(fee_per_output=dust_tx_fee, version=tx_version)

rkrux commented at 10:27 am on November 26, 2024:

0fee_per_output=dust_tx_fee

Since technically it’s the fee per output, let’s call it dust_tx_fee_per_output? Otherwise reading the function signatures gives an impression that dust_tx_fee is for the whole tx.

theStack commented at 12:54 pm on November 26, 2024:

Note that this fee is set initially for a tx with a single (non-dust) output, and any additional outputs after are created in a way that the total tx fee stays constant (see doc-string of function add_output_to_create_multi_result: Add output without changing absolute tx fee). So the parameter denotes indeed the fee for the whole tx.

rkrux commented at 7:45 am on November 28, 2024:

Oh interesting, thanks for the clarification. It was seeing fee_per_output=dust_tx_fee together that threw me off. Maybe an argument comment here at this call site of create self transfer multi can be more clarifying stating that there is only 1 output created initially? That is if the file is retouched.

in test/functional/mempool_ephemeral_dust.py:50 in 160799d913

46@@ -47,6 +47,22 @@ def add_output_to_create_multi_result(self, result, output_value=0):
47 
48         result["new_utxos"].append({"txid": new_txid, "vout": len(result["tx"].vout) - 1, "value": Decimal(output_value) / COIN, "height": 0, "coinbase": False, "confirmations": 0})
49 
50+    def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsors=None):

rkrux commented at 10:28 am on November 26, 2024:

0- def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsors=None):
1+ def create_ephemeral_dust_package(self, *, tx_version=3, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsors=None):

Because v3 is the most common in the file.

rkrux commented at 10:29 am on November 26, 2024:

0- def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsors=None):
1+ def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsor_coins=None):

Nit for verbosity in function signature.

theStack commented at 12:57 pm on November 26, 2024:

Hmm, thought about that, but I think this could be confusing, especially considering that transactions are still created with nVersion=2 by default (see e.g. #31348). Seems more expressive to be explicit about the tx version.

theStack commented at 12:57 pm on November 26, 2024:

Will do if I have to retouch.

rkrux commented at 7:30 am on November 28, 2024:

Ah I see, makes sense to me.

fanquake requested review from instagibbs on Nov 26, 2024

rkrux approved

rkrux commented at 10:41 am on November 26, 2024: none

tACK 160799d9135528dbdea40690f0bb0d56c6c4803a

I agree with this change, gets rid of duplication. Successful make and functional tests.

I noticed 3 call sites at the end where the returned sweep_tx is not used, though it seems redundant but I don’t suppose it’s a big deal, just noteworthy.

Left few minor suggestions.

in test/functional/mempool_ephemeral_dust.py:51 in 160799d913

46@@ -47,6 +47,22 @@ def add_output_to_create_multi_result(self, result, output_value=0):
47 
48         result["new_utxos"].append({"txid": new_txid, "vout": len(result["tx"].vout) - 1, "value": Decimal(output_value) / COIN, "height": 0, "coinbase": False, "confirmations": 0})
49 
50+    def create_ephemeral_dust_package(self, *, tx_version, dust_tx_fee=0, dust_value=0, num_dust_outputs=1, extra_sponsors=None):
51+        """Creates a 1P1C package containing ephemeral dust. By default, the parent transaction

instagibbs commented at 1:55 pm on November 26, 2024:

micro-nit, no need to invalidate acks

0        """Creates a 1P1C package containing ephemeral dust by default. By default, the parent transaction

instagibbs approved

instagibbs commented at 2:00 pm on November 26, 2024: member

ACK 160799d9135528dbdea40690f0bb0d56c6c4803a

tdb3 approved

tdb3 commented at 1:39 am on November 28, 2024: contributor

Code review ACK 160799d9135528dbdea40690f0bb0d56c6c4803a

Noticed that mempool_ephemeral_dust.py takes a significant amount of time to run (around 50s on at least on two of my machines, i.e. greater than 30s), but is near the bottom of BASE_SCRIPTS in test_runner.py. Moving the test toward the front of the list significantly improved parallel test runtime on each of my machines (e.g. from 131s to 91s, jobs=32). ymmv. I’m happy to submit a separate PR (https://github.com/tdb3/bitcoin/commit/3c40aac84972e0887ecd41161231a08f87d1efa3) to prevent ACK invalidation.

 0diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
 1index 701d81b9d25..c617ecd368d 100755
 2--- a/test/functional/test_runner.py
 3+++ b/test/functional/test_runner.py
 4@@ -102,6 +102,7 @@ BASE_SCRIPTS = [
 5     'mining_getblocktemplate_longpoll.py',
 6     'p2p_segwit.py',
 7     'feature_maxuploadtarget.py',
 8+    'mempool_ephemeral_dust.py',
 9     'feature_assumeutxo.py',
10     'mempool_updatefromblock.py',
11     'mempool_persist.py --descriptors',
12@@ -400,7 +401,6 @@ BASE_SCRIPTS = [
13     'rpc_getdescriptorinfo.py',
14     'rpc_mempool_info.py',
15     'rpc_help.py',
16-    'mempool_ephemeral_dust.py',
17     'p2p_handshake.py',
18     'p2p_handshake.py --v2transport',
19     'feature_dirsymlinks.py',

theStack commented at 11:12 am on November 28, 2024: contributor

Noticed that mempool_ephemeral_dust.py takes a significant amount of time to run (around 50s on at least on two of my machines, i.e. greater than 30s), but is near the bottom of BASE_SCRIPTS in test_runner.py. Moving the test toward the front of the list significantly improved parallel test runtime on each of my machines (e.g. from 131s to 91s, jobs=32). ymmv. I’m happy to submit a separate PR (tdb3@3c40aac) to prevent ACK invalidation.

Good idea, will include that if I have to retouch. In a follow-up PR, I think it could make sense to check the run-time of other functional tests as well and reorder them if necessary?

tdb3 commented at 12:50 pm on November 28, 2024: contributor

I think it could make sense to check the run-time of other functional tests as well and reorder them if necessary?

Yes, I think it’s worth a deeper look. I did use test_runner.py --resultsfile=runtimes.csv and sorted by time. Did some rearranging, but was only able to shave off an additional 3% or so. That was just a quick/rough attempt though.

instagibbs commented at 12:58 pm on November 28, 2024: member

Part of the cost of the test is ensuring that 1P1C relay is working properly. The merged ephemeral dust code is a bit more straightforward as 0-fee requirement is enforced only so maybe it’s not as necessary?

On Thu, Nov 28, 2024, 7:50 AM tdb3 @.***> wrote:

I think it could make sense to check the run-time of other functional tests as well and reorder them if necessary?

Yes, I think it’s worth a deeper look. I did use test_runner.py –resultsfile=runtimes.csv and sorted by time. Did some rearranging, but was only able to shave off an additional 3% or so. That was just a quick/rough attempt though.

— Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/31371#issuecomment-2506047798, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFUY5HZ222FPWIA46NHT2C4GSZAVCNFSM6AAAAABSO2I3DSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBWGA2DONZZHA . You are receiving this because your review was requested.Message ID: @.***>

glozow merged this on Nov 29, 2024

glozow closed this on Nov 29, 2024

theStack deleted the branch on Nov 29, 2024

doc, test: more ephemeral dust follow-ups #31371

Code Coverage & Benchmarks

Reviews

Conflicts