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

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202411-test-introduce_create_ephemeral_dust_package-helper changing 2 files +31 −64
  1. 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.

  2. doc: ephemeral policy: add missing closing double quote 61e18dec30
  3. 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.

  4. test: refactor: introduce `create_ephemeral_dust_package` helper 160799d913
  5. theStack force-pushed on Nov 25, 2024
  6. DrahtBot added the label CI failed on Nov 25, 2024
  7. DrahtBot removed the label CI failed on Nov 25, 2024
  8. 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.
  9. 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.
  10. fanquake requested review from instagibbs on Nov 26, 2024
  11. rkrux approved
  12. 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.

  13. 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
    
  14. instagibbs approved
  15. instagibbs commented at 2:00 pm on November 26, 2024: member
    ACK 160799d9135528dbdea40690f0bb0d56c6c4803a
  16. tdb3 approved
  17. 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',
    
  18. 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?

  19. 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.

  20. 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: @.***>

  21. glozow merged this on Nov 29, 2024
  22. glozow closed this on Nov 29, 2024

  23. theStack deleted the branch on Nov 29, 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-12-03 18:12 UTC

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