Add test for send{toaddress, many} conf_target under subtractfromamount scenario

instagibbs commented at 8:57 pm on September 26, 2019: member

This may be a weird spot for a test but wanted to try and catch #16072 which the test still doesn’t seem to trigger.

instagibbs force-pushed on Sep 26, 2019

DrahtBot added the label Tests on Sep 26, 2019

instagibbs force-pushed on Sep 26, 2019

Sjors commented at 3:51 pm on September 27, 2019: member

Concept ACK. Could add a test for knapsack vs. branch and bound solution, if you can get the coins organized for that.

instagibbs commented at 5:13 pm on September 27, 2019: member

Yes I was thinking about some test-only hidden RPC where you can hack block estimates to be some function you provide. Then we can put this test somewhere nicer and do more tests like this

instagibbs marked this as ready for review on Sep 27, 2019

in test/functional/feature_fee_estimation.py:254 in 24bc1a2174 outdated

249+        assert high_feerate["feerate"]/2 > low_feerate["feerate"] # comfortably larger
250+        assert high_feerate["blocks"] < low_feerate["blocks"]
251+
252+        # Spend all coins at once
253+        self.nodes[1].sendtoaddress(self.nodes[1].getnewaddress(), self.nodes[1].getbalance(), "", "", True, True, low_count, "ECONOMICAL")
254+       # Then re-spend the change a couple times to test close as possible scenarios

laanwj commented at 8:13 am on October 2, 2019:

indentation error

in test/functional/feature_fee_estimation.py:263 in 24bc1a2174 outdated

258+        low_info = self.nodes[1].getmempoolentry(low_txid)
259+        high_info = self.nodes[1].getmempoolentry(high_txid)
260+
261+        # Total fee should be really close
262+        assert abs(low_info["vsize"]*low_feerate["feerate"]/1000 - low_info["fee"]) < 0.00000001
263+        assert abs(high_info["vsize"]*high_feerate["feerate"]/1000 - high_info["fee"]) < 0.00000001

MarcoFalke commented at 9:11 pm on October 10, 2019:

should use a helper like assert_approx to print the values in case of failure

in test/functional/feature_fee_estimation.py:256 in 24bc1a2174 outdated

251+
252+        # Spend all coins at once
253+        self.nodes[1].sendtoaddress(self.nodes[1].getnewaddress(), self.nodes[1].getbalance(), "", "", True, True, low_count, "ECONOMICAL")
254+       # Then re-spend the change a couple times to test close as possible scenarios
255+        low_txid = self.nodes[1].sendtoaddress(self.nodes[1].getnewaddress(), self.nodes[1].getbalance(), "", "", True, True, low_count, "ECONOMICAL")
256+        high_txid = self.nodes[1].sendtoaddress(self.nodes[1].getnewaddress(), self.nodes[1].getbalance(), "", "", True, True, high_count, "ECONOMICAL")

MarcoFalke commented at 9:12 pm on October 10, 2019:

should use named args for unnamed integer types and bools

instagibbs force-pushed on Aug 10, 2020

instagibbs commented at 7:04 pm on August 10, 2020: member

all comments addressed (and rebased on master since it’s so ancient)

instagibbs force-pushed on Aug 10, 2020

MarcoFalke commented at 6:11 am on August 11, 2020: member

It looks like you are sending the whole balance. Maybe also add a test for the case “The sum of my specified output amounts was exactly equal to the value of the single unlocked UTxO in my wallet.” (https://github.com/bitcoin/bitcoin/issues/16072#issuecomment-671668011). No strong opinion though.

Big Concept ACK on adding the test

instagibbs commented at 1:15 pm on August 11, 2020: member

@MarcoFalke The test first pipes all utxos into one, then does exactly that(although there’s literally only one wallet utxo left).

One thing I can test is sendmany with subtractfeefromoutput index working properly since that’s the latest report

instagibbs force-pushed on Aug 11, 2020

Add test for send{toaddress, many} conf_target under subtractfromamount scenario c099cf502b

instagibbs force-pushed on Aug 11, 2020

instagibbs renamed this:
~~Add test for sendtoaddress conf_target under subtractfromamount scenario~~
Add test for send{toaddress, many} conf_target under subtractfromamount scenario
on Aug 11, 2020

in test/functional/feature_fee_estimation.py:291 in c099cf502b

286+        low_many_info = self.nodes[1].getmempoolentry(low_many_txid)
287+        high_many_info = self.nodes[1].getmempoolentry(high_many_txid)
288+
289+        assert_approx(low_info["vsize"]*low_feerate["feerate"]/1000, float(low_info["fee"]))
290+        assert_approx(high_info["vsize"]*high_feerate["feerate"]/1000, float(high_info["fee"]))
291+        assert_approx(low_many_info["vsize"]*low_feerate["feerate"]/1000, float(low_many_info["fee"]))

MarcoFalke commented at 9:12 am on August 30, 2020:

fails

https://travis-ci.org/github/bitcoin/bitcoin/jobs/716915333#L8855

instagibbs commented at 7:29 pm on August 30, 2020:

hmm swore I ran it locally, looks like it’s just outside the approx range. I’ll do some log-hunting to make sure nothing is especially weird and relax the range a bit if not.

instagibbs commented at 1:51 pm on August 31, 2020:

Seems like a really internal piece of fee estimation machinery difference that only happens every other time:

02020-08-31T13:47:22.928489Z [httpworker.3] [default wallet] Fee Calculation: Fee:17062 Bytes:208 Needed:17062 Tgt:2 (requested 2) Reason:"Double Target 95% Threshold" Decay 0.96200: Estimation: (80730.4 - 93455.5) 97.69% 28.8/(29.4 0 mem 0.0 out) Fail: (76886.1 - 80730.4) 93.58% 39.6/(42.3 0 mem 0.0 out)

vs

0 node1 2020-08-11T14:14:23.348964Z [default wallet] Fee Calculation: Fee:23514 Bytes:208 Needed:23514 Tgt:2 (requested 2) Reason:"Target 85% Threshold" Decay 0.96200: Estimation: (108186 - 113596) 91.18% 38.1/(41.8 0 mem 0.0 out) Fail: (93455.5 - 108186) 80.72% 50.3/(62.4 0 mem 0.0 out)

doesn’t explain why the estimatesmartfee returns something different… need to investigate more sometime

Normally I’d say this difference was because change was being tossed to miner fee, but this cannot happen in this case.

aarmoa commented at 1:03 pm on March 21, 2021: none

I tried testing the change, but I never got to execute the new validations because the test fails after starting. It seems like in line 210 it is trying to split an empty list of unspent inputs.

2021-03-21T12:59:24.685000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_bsz8pvt2 2021-03-21T12:59:26.988000Z TestFramework (INFO): This test is time consuming, please be patient 2021-03-21T12:59:26.988000Z TestFramework (INFO): Splitting inputs so we can generate tx’s 2021-03-21T12:59:27.497000Z TestFramework (ERROR): Unexpected exception caught during testing Traceback (most recent call last): File “/bitcoin_core/bitcoin/test/functional/test_framework/test_framework.py”, line 118, in main self.run_test() File “/bitcoin_core/bitcoin/test/functional/feature_fee_estimation.py”, line 210, in run_test split_inputs(self.nodes[0], self.nodes[0].listunspent(0), self.txouts, True) File “/bitcoin_core/bitcoin/test/functional/feature_fee_estimation.py”, line 84, in split_inputs prevtxout = txins.pop() IndexError: pop from empty list 2021-03-21T12:59:27.548000Z TestFramework (INFO): Stopping nodes 2021-03-21T12:59:27.850000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_bsz8pvt2 2021-03-21T12:59:27.850000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_bsz8pvt2/test_framework.log 2021-03-21T12:59:27.850000Z TestFramework (ERROR): 2021-03-21T12:59:27.851000Z TestFramework (ERROR): Hint: Call /home/abel/projects/bitcoin_core/bitcoin/test/functional/combine_logs.py ‘/tmp/bitcoin_func_test_bsz8pvt2’ to consolidate all logs 2021-03-21T12:59:27.851000Z TestFramework (ERROR): 2021-03-21T12:59:27.851000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 2021-03-21T12:59:27.851000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues 2021-03-21T12:59:27.851000Z TestFramework (ERROR):

Process finished with exit code 1

Tested in Linux Mint 20 Cinnamon

instagibbs commented at 4:39 am on March 22, 2021: member

closing due to inattention on my part, please put it up for grabs

instagibbs closed this on Mar 22, 2021

fanquake added the label Up for grabs on Mar 22, 2021

DrahtBot locked this on Aug 16, 2022

Add test for send{toaddress, many} conf_target under subtractfromamount scenario #16972