test: add coverage for dust mempool policy (-dustrelayfee setting) #26631

1. 
   theStack commented at 2:46 am on December 4, 2022: contributor

   This PR adds missing test coverage for the -dustrelayfee setting, which specifies the fee-rate used to define dust. Output scripts for all common types that are treated as standard by default (P2PK, P2(W)PKH, P2(W)SH, P2TR, bare multisig, null data, unknown witness versions v2+) are created and then checked for dust-mempool-policy each via the testmempoolaccept RPC: a tx with an output’s nValue equal to the dust threshold should be accepted, one with an nValue of just one 1 satoshi below that should be rejected with reason dust. This is repeatedly done for a fixed (but obviously somewhat arbitrary) list of different -dustrelayfee settings on a single node, including the default and zero (i.e. no dust limit) settings.

   Note that the first commit introduces a necessary CScript helper method IsWitnessProgram (using PascalCase in Python is likely controversial; in this case the style for the already existing method GetSigOpCount was followed, which also refers to a method in the core CScript class).

   Some historical information about dust, contributed by pablomartin4btc: “The concept of dust was first introduced in #2577. This commit from #9380 introduced the -dustrelayfee option. Previous to that PR, the dust feerate was whatever -minrelaytxfee was set to.”

2. test: add `CScript` method for checking for witness program

   This is needed in the next commit to calculate the dust threshold
   for a given output script and min feerate for defining dust.

   8a5dbe2879
3. 
   DrahtBot commented at 2:46 am on December 4, 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	glozow, kouloumos, LarryRuane
   Concept ACK	pablomartin4btc, rserranon, instagibbs

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Coverage

   Coverage	Change (pull 26631, 8147f7f7ec5dcfe5295ed2697942b410554b4117)	Reference (master, 5055d07edf4625d2)
   Lines	+0.0036 %	83.9288 %
   Functions	+0.0000 %	80.9329 %
   Branches	+0.0040 %	51.4098 %

   Updated at: 2022-12-16T12:29:44.053355.

4. DrahtBot added the label Tests on Dec 4, 2022
5. theStack force-pushed on Dec 4, 2022
6. theStack force-pushed on Dec 4, 2022
7. 
   glozow commented at 5:22 pm on December 5, 2022: member

   Concept ACK
8. in test/functional/test_framework/script.py:600 in 2c697ddcb1 outdated

   596@@ -597,6 +597,13 @@ def GetSigOpCount(self, fAccurate):
   597             lastOpcode = opcode
   598         return n
   599 
   600+    def IsWitnessProgram(self):
   

   ---

   ---

   

   LarryRuane commented at 10:49 pm on December 11, 2022:

   maybe add a comment before this definition:

   0# Equivalent to: CScript::IsWitnessProgram()
   

9. in test/functional/mempool_dust.py:97 in 2c697ddcb1 outdated

   92+            (program_to_witness_script(16, b'\x77' * 40), "P2?? (future witness version 16)"),
   93+            (keys_to_multisig_script([pubkey]*3),         "bare multisig (3-of-3)"),
   94+            (CScript([OP_RETURN, b'superimportanthash']), "null data (OP_RETURN)"),
   95+        ]
   96+
   97+        for dustfee_sat_kvb in [DUST_RELAY_TX_FEE, 0, 1, 66, 500, 1337, 12345, 21212]:
   

   ---

   ---

   

   LarryRuane commented at 6:31 am on December 12, 2022:

   nit

   0        for dustfee_sat_kvb in (DUST_RELAY_TX_FEE, 0, 1, 66, 500, 1337, 12345, 21212):
   

   ---

   

   rserranon commented at 5:11 am on December 14, 2022:

   Should a comment be added with a note that -dustrelayfee list elements are somewhat arbitrary?, or is there a rationale that should be mentioned?

   ---

   

   theStack commented at 2:06 pm on December 16, 2022:

   Good idea, added a comment above that for-loop in the latest force-push ("# test default (no parameter), disabled (=0) and a bunch of arbitrary dust fee rates [sat/kvB]").

   ---

   

   theStack commented at 2:09 pm on December 16, 2022:

   Done, also for the output scripts.
10. in test/functional/mempool_dust.py:56 in 2c697ddcb1 outdated

    51+            tx_size += 67 if output_script.IsWitnessProgram() else 148
    52+            dust_threshold = int(get_fee(tx_size, dust_relay_fee) * COIN)
    53+        self.log.info(f"-> Test {type_desc} output (size {len(output_script)}, limit {dust_threshold})")
    54+
    55+        # amount right on the dust threshold should pass
    56+        tx = self.wallet.create_self_transfer()["tx"]
    

    ---

    ---

    

    LarryRuane commented at 6:04 pm on December 12, 2022:

    Not actually needed, but just to make sure …

    0        tx = self.wallet.create_self_transfer()["tx"]
    1        # make sure there's enough value available for the new output we're creating
    2        tx.vout[0].nValue -= dust_threshold
    

    Note that since this tx is spending coinbase, there’s almost 50BTC available in the first output, and also spending coinbase doesn’t require a signature here in the functional tests, so we can make this adjustment without having to re-signing the transaction.

    ---

    

    theStack commented at 2:23 pm on December 16, 2022:

    Thanks, done. In fact after extending the dust fee-rate test list with a larger value (333333), the test would fail without this change, so it was very helpful. Note that not requiring a signature is not only true for the coinbase outputs we spend here, but also to outputs created by MiniWallet going to the exact same address.
11. 
    LarryRuane commented at 6:06 pm on December 12, 2022: contributor

    ACK 2c697ddcb1fa4d04f55b024c06116530d7e74598 suggestions are non-blocking
12. 
    pablomartin4btc commented at 0:25 am on December 14, 2022: member

    Tested ACK.

    

    All different output scripts ran over the -dustrelayfee list of settings as it’s shown on the screenshot above.

13. 
    rserranon commented at 4:27 pm on December 14, 2022: none

    Tested ACK
14. in test/functional/mempool_dust.py:92 in 2c697ddcb1 outdated

    87+            (script_to_p2sh_script(CScript([OP_TRUE])),   "P2SH"),
    88+            (key_to_p2wpkh_script(pubkey),                "P2WPKH"),
    89+            (script_to_p2wsh_script(CScript([OP_TRUE])),  "P2WSH"),
    90+            (output_key_to_p2tr_script(pubkey[1:]),       "P2TR"),
    91+            (program_to_witness_script(2,  b'\x66' * 21), "P2?? (future witness version 2)"),
    92+            (program_to_witness_script(16, b'\x77' * 40), "P2?? (future witness version 16)"),
    

    ---

    ---

    

    rserranon commented at 4:53 pm on December 14, 2022:

    Can you explain the rationale for using 21 bytes and 40 bytes for future witness versions 2 and 16 respectively? Maybe a comment with the standard size for each Script type could be useful for testers and people learning.

    ---

    

    theStack commented at 2:13 pm on December 16, 2022:

    It could be any size between 2 and 40 bytes. Added a corresponding comment in latest force-push ("# witness programs for segwitv2+ can be between 2 and 40 bytes") and adapted the values accordingly. I prefer now having one with 2 bytes instead of 21 bytes (which was indeed completely arbitrary) as it is the smallest possible output script that is considered standard.
15. 
    pablomartin4btc commented at 3:59 pm on December 15, 2022: member

    I think this statement is very useful as a context for the review:

    The concept of dust was first introduced in [PR #2577](https://github.com/bitcoin/bitcoin/pull/2577). This commit from [PR #9380](https://github.com/bitcoin/bitcoin/pull/9380) introduced the -dustrelayfee option. Previous to that PR, the dust feerate was whatever -minrelaytxfee was set to.

    Taken from the notes of the review club. Is it possible to add the label “Review club” to this PR please?

16. test: add coverage for dust mempool policy (`-dustrelayfee` setting) d6fc1d6a33
17. theStack force-pushed on Dec 16, 2022
18. 
    theStack commented at 2:29 pm on December 16, 2022: contributor

    Force-pushed with most suggestions taken, and another few changes:

    • add both smallest and largest possible (non-nulldata) output scripts that are considered standard to the list (i.e. segwitv2+ with 2-byte witness program, m-of-3 bare multisig with uncompressed pubkeys)
    • extended the dust fee-rate list to test with 333333, to also have one case covering the “~hundreds of sats/vbyte” range
    • add comments about program witness sizes and chosen dust fee-rates to reduce confusion about magic numbers
    • for tx creation: adjust first output with the dust threshold amount, to keep the total output value constant and not risk underpaying fees (or even worse, exceeding the input amount)

    I think this statement is very useful as a context for the review:

    The concept of dust was first introduced in [PR #2577](https://github.com/bitcoin/bitcoin/pull/2577). This commit from [PR #9380](https://github.com/bitcoin/bitcoin/pull/9380) introduced the -dustrelayfee option. Previous to that PR, the dust feerate was whatever -minrelaytxfee was set to.

    Thanks for researching! 👍 I added this paragraph to the PR description, as it is very helpful for reviewers getting deeper into the topic of dust with some historical context.

    Taken from the notes of the review club. Is it possible to add the label “Review club” to this PR please?

    +1

19. maflcko added the label Review club on Dec 18, 2022
20. in test/functional/mempool_dust.py:44 in d6fc1d6a33

    39+
    40+class DustRelayFeeTest(BitcoinTestFramework):
    41+    def set_test_params(self):
    42+        self.num_nodes = 1
    43+
    44+    def test_dust_output(self, node: TestNode, dust_relay_fee: Decimal,
    

    ---

    ---

    

    kouloumos commented at 8:29 am on January 12, 2023:

    the node argument may be residue from when the PR used multiple nodes to test different configurations?
21. kouloumos referenced this in commit 3bb557f662 on Jan 12, 2023
22. 
    glozow commented at 2:01 pm on January 13, 2023: member

    ACK d6fc1d6a3393c571a1691a6bda60433216643616 dust calculation seems to match GetDustThreshold(), IsWitnessProgram() looks correct, don’t see any off-by-ones, coverage looks good
23. in test/functional/mempool_dust.py:53 in d6fc1d6a33

    48+            dust_threshold = 0
    49+        else:
    50+            tx_size = len(CTxOut(nValue=0, scriptPubKey=output_script).serialize())
    51+            tx_size += 67 if output_script.IsWitnessProgram() else 148
    52+            dust_threshold = int(get_fee(tx_size, dust_relay_fee) * COIN)
    53+        self.log.info(f"-> Test {type_desc} output (size {len(output_script)}, limit {dust_threshold})")
    

    ---

    ---

    

    kouloumos commented at 2:43 pm on January 13, 2023:

    nit: I’ve been thinking about ways to formalize the way logging is done in the testing framework and indentation of subcases is in there. The ideal scenario would be for those things to happen automatically, but working with what we have… I would like to make a minor suggestion that I believe makes the logging more readable:

    0        self.log.info(f" - Test {type_desc} output (size {len(output_script)}, limit {dust_threshold})")
    

    side by side comparison

    and to remove even more noise:

    0        self.log.info(f"- output (size {len(output_script)}, limit {dust_threshold})")
    

    

    I’ve noticed that this is a format that you have previously used in wallet_fast_rescan.py, but “- …” is currently used in more place, such as rpc_decodescript.py and rpc_psbt.py and I believe that following the same format across the testing framework is for the better.

24. in test/functional/mempool_dust.py:93 in d6fc1d6a33

    88+            (script_to_p2sh_script(CScript([OP_TRUE])),        "P2SH"),
    89+            (key_to_p2wpkh_script(pubkey),                     "P2WPKH"),
    90+            (script_to_p2wsh_script(CScript([OP_TRUE])),       "P2WSH"),
    91+            (output_key_to_p2tr_script(pubkey[1:]),            "P2TR"),
    92+            # witness programs for segwitv2+ can be between 2 and 40 bytes
    93+            (program_to_witness_script(2,  b'\x66' * 2),       "P2?? (future witness version 2)"),
    

    ---

    ---

    

    instagibbs commented at 3:15 pm on January 13, 2023:

    As I do in #26875 could check v1 at non-32-byte-push lengths as well to make sure they’re treated as UNKNOWN
25. 
    instagibbs commented at 3:16 pm on January 13, 2023: member

    concept ACK
26. 
    kouloumos commented at 4:11 pm on January 13, 2023: contributor

    ACK d6fc1d6a3393c571a1691a6bda60433216643616 with a few non-blocking comments
27. 
    LarryRuane commented at 6:06 pm on January 13, 2023: contributor

    ACK d6fc1d6a3393c571a1691a6bda60433216643616
28. maflcko merged this on Jan 16, 2023
29. maflcko closed this on Jan 16, 2023

    ---

30. theStack deleted the branch on Jan 16, 2023
31. sidhujag referenced this in commit 07930b5e95 on Jan 16, 2023
32. bitcoin locked this on Jan 17, 2024

Contributors
theStack DrahtBot glozow LarryRuane rserranon pablomartin4btc kouloumos instagibbs

Labels
Tests Review club