test: fix misleading fee unit in mempool_limit.py #22972

1. 
   theStack commented at 2:08 pm on September 14, 2021: member

   The PR is a follow-up to #22543. The helper send_large_txs in its current interface has a fee_rate parameter, implying that it would create a transaction with exactly that rate. Unfortunately, this fee rate is only passed to MiniWallet’s create_self_transfer method, which can’t know that we append several tx outputs after, increasing the tx’s vsize and decreasing it’s fee rate accordingly.

   In our case, the fee rate is off by several orders of magnitude, as the tx’s vsize changes changes from 96 to 67552 vbytes (>700x), i.e. the value passed to this function is neither really a fee rate nor an absolute fee, but something in-between, which is very confusing. It was suggested to simply in-line this helper as it’s currently only used in this single test (https://github.com/bitcoin/bitcoin/pull/22543#discussion_r701685136, #22543 (comment)), but I could imagine that this helper may also become useful for other tests and may be moved to a library (e.g. wallet.py) in the future.

   Clarify the interface by passing an absolute fee that is deducted in the end (and also verified, via testmempoolaccept) and also describe how we come up with the value passed. On master, the comment says that the fee rate needs to increased “massively”; this word is also removed because the fee rate only needs to be higher for the test to succeed.

2. test: fix misleading fee unit in mempool_limit.py

   The helper `send_large_txs` in its current interface has a fee_rate
   parameter, implying that it would create a transaction with exactly that
   rate. Unfortunately, this fee rate is only passed to MiniWallet's
   `create_self_transfer` method, which can't know that we append several
   tx outputs after, increasing the tx's vsize and decreasing it's fee rate
   accordingly.
   
   In our case, the fee rate is off by several orders of magnitude, as the
   tx's vsize changes changes from 96 to 67552 vbytes (>700x), i.e. the
   value passed to this function is neither really a fee rate nor an
   absolute fee, but something in-between, which is very confusing.
   
   Clarify the interface by passing an absolute fee that is deducted in the end
   (and verified, via testmempoolaccept) and also describe how we come up with the
   value passed.

   2600db6c36
3. 
   Zero-1729 commented at 2:17 pm on September 14, 2021: contributor

   Concept ACK
4. DrahtBot added the label Tests on Sep 14, 2021
5. in test/functional/mempool_limit.py:34 in 2600db6c36

   28@@ -23,16 +29,19 @@ def set_test_params(self):
   29         ]]
   30         self.supports_cli = False
   31 
   32-    def send_large_txs(self, node, miniwallet, txouts, fee_rate, tx_batch_size):
   33+    def send_large_txs(self, node, miniwallet, txouts, fee, tx_batch_size):
   34         for _ in range(tx_batch_size):
   35-            tx = miniwallet.create_self_transfer(from_node=node, fee_rate=fee_rate)['tx']
   36+            tx = miniwallet.create_self_transfer(from_node=node, fee_rate=0, mempool_valid=False)['tx']
   

   ---

   ---

   

   michaelfolkson commented at 10:18 am on October 4, 2021:

   Should this fee_rate be replaced by fee here too? No, create_self_transfer has a fee_rate argument
6. 
   michaelfolkson commented at 10:25 am on October 4, 2021: contributor

   Concept ACK

   the value passed to this function is neither really a fee rate nor an absolute fee

   Definitely seems optimal to exclusively use absolute fee and fee rate rather than having some hard to define third variable

7. 
   stratospher commented at 5:34 pm on October 28, 2021: contributor

   ACK 2600db6.

   This change clarifies the problem with the fee rates in the test.

8. MarcoFalke merged this on Oct 29, 2021
9. MarcoFalke closed this on Oct 29, 2021

   ---

10. theStack deleted the branch on Oct 29, 2021
11. sidhujag referenced this in commit 1d861909f5 on Oct 29, 2021
12. DrahtBot locked this on Oct 30, 2022

Contributors
theStack Zero-1729 michaelfolkson stratospher

Labels
Tests