Some cleanups that i noticed would be desirable while working on #23074 and #22539, which are intentionally not based on it. Mainly simplifications and a slight speedup.
2**9 coins instead of creating 2**8 2-outputs transactions
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
initial_split function could be moved to the MiniWallet since other tests could benefit from it too.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
139@@ -158,8 +140,10 @@ def send_tx(node, utxo, feerate):
140 fee = tx_size * feerate
141
142 tx = CTransaction()
143- tx.vin = [CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), SCRIPT_SIG[utxo["vout"]])]
144- tx.vout = [CTxOut(int(utxo["amount"] * COIN) - fee, P2SH_1)]
145+ tx.vin = [
146+ CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), REDEEM_SCRIPT)
147+ ]
148+ tx.vout = [CTxOut(int(utxo["amount"] * COIN) - fee, P2SH)]
142- tx.vout = [CTxOut(int(utxo["amount"] * COIN) - fee, P2SH)]
143- txid = node.sendrawtransaction(tx.serialize().hex())
144+ tx.vout = [CTxOut(int(utxo["amount"] * COIN), P2SH)]
145+
146+ # vbytes == bytes as we are using legacy transactions
147+ fee = len(tx.serialize()) * feerate
in 7ccd775dcc5f363c9f35409e9170b1e07bfdc753:
Why not just use tx.get_vsize() ?
SCRIPT_SIG
Also seems like you tried to co-author
The issue is that Co-Atuhored-By: is spelt wrong.
Using 2 different scripts is unnecessary complication
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
This simplifies the code, and slightly speeds up the test.
Running `./test/functional/test_runner.py -j15 $(printf 'feature_fee_estimation %.0s' {1..15})`
on master 3 times gives:
- Before:
ALL | ✓ Passed | 788 s (accumulated)
ALL | ✓ Passed | 818 s (accumulated)
ALL | ✓ Passed | 873 s (accumulated)
- After:
ALL | ✓ Passed | 763 s (accumulated)
ALL | ✓ Passed | 798 s (accumulated)
ALL | ✓ Passed | 731 s (accumulated)
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
We don't need dust outputs anymore.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Followups to #22539
Co-Authored-By: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
228+ self.confutxo = [
229+ {"txid": txid, "vout": i, "amount": splitted_amount}
230+ for i in range(utxo_count)
231+ ]
232 while len(node.getrawmempool()) > 0:
233 self.generate(node, 1, sync_fun=self.no_op)
Is the node “state” here expected to exactly the same before and after changes in https://github.com/bitcoin/bitcoin/pull/23075/commits/1fc03155e53f4f0a7f0a2529e55e802f49260b23? If so, how to test?
E.g. this is what I tried to compare:
node.getbalance() returns Decimal('1200.00000000')
len(node.listunspent()) returns 24
node.getbalance() returns Decimal('1150.00000000')
len(node.listunspent()) returns 23
Is the node “state” here expected to exactly the same before and after changes in 1fc0315?
It’s not. At least, it was not my intention.
148+ tx.vout = [CTxOut(int(utxo["amount"] * COIN), P2SH)]
149+
150+ # vbytes == bytes as we are using legacy transactions
151+ fee = tx.get_vsize() * feerate
152+ tx.vout[0].nValue -= fee
153
fee = tx.get_vsize() * feerate to calculate fee?
Thank you for the pr. As a newcomer, I find it very educational to review.
I ACK all commits up to https://github.com/bitcoin/bitcoin/pull/23075/commits/60ae1161a4c415cc73f47df95598f3688e8d34df (except https://github.com/bitcoin/bitcoin/pull/23075/commits/1fc03155e53f4f0a7f0a2529e55e802f49260b23, where I have a question more for my own learning than actually questioning the PR). I built and ran the test successfully. I agree after the changes, the behavior is kept the same and the code is shorter and easier to reason.
145@@ -146,12 +146,11 @@ def send_tx(node, utxo, feerate):
146 class EstimateFeeTest(BitcoinTestFramework):
147 def set_test_params(self):
148 self.num_nodes = 3
149- # mine non-standard txs (e.g. txs with "dust" outputs)
150 # Force fSendTrickle to true (via whitelist.noban)
151 self.extra_args = [
152- ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1"],
153- ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1", "-blockmaxweight=68000"],
154- ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1", "-blockmaxweight=32000"],
-acceptnonstdtxn
utACK 60ae1161a4c415cc73f47df95598f3688e8d34df
Light code re-review, commits all pass now. Thanks for addressing comments, sorry for the delay!
39-REDEEM_SCRIPT_2 = CScript([OP_2, OP_DROP])
40-P2SH_1 = script_to_p2sh_script(REDEEM_SCRIPT_1)
41-P2SH_2 = script_to_p2sh_script(REDEEM_SCRIPT_2)
42+SCRIPT = CScript([OP_1, OP_DROP])
43+P2SH = script_to_p2sh_script(SCRIPT)
44+REDEEM_SCRIPT = CScript([OP_TRUE, SCRIPT])
I think the previous wording was correct. Redeem script refers to the thing that is hashed (serialized), see also BIP 16:
serialized script - also referred to as the redeemScript