test: Refactor rpc_fundrawtransaction.py #25291

akankshakashyap commented at 10:44 AM on June 7, 2022: contributor

Made 2 changes to tests/functional/rpc_fundrawtransaction.py-

Remove test_simple_two_coins - The current test_simple_two_coins and test_simple are identical and redundant. The test_simple_two_coins was changed in #6417 and asserts ensuring the use of two input coins were removed.
Use changepos to get the change output - In test_address_reuse the code to get the change output assumes that a coin with a value > 2 BTC was used to fund the transaction, and therefore change value will be greater than 1 BTC. This can fail if previous tests add a small coin. fundrawtransaction directly gives the index of change output as changepos, we use this to get the change output more reliably and cleanly.

akankshakashyap renamed this:
~~Refactor rpc_fundrawtransaction.py~~
test: Refactor rpc_fundrawtransaction.py
on Jun 7, 2022

akankshakashyap marked this as ready for review on Jun 7, 2022

laanwj added the label Tests on Jun 7, 2022

in test/functional/rpc_fundrawtransaction.py:114 in 1bddea821e outdated

 102 | @@ -103,9 +103,15 @@ def run_test(self):
 103 |          self.generate(self.nodes[2], 1)
 104 |          self.generate(self.nodes[0], 121)
 105 |  
 106 | +        # coins for simple tests
 107 | +        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.5)
 108 | +        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.0)
 109 | +        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 5.0)
 110 | +
 111 | +        self.generate(self.nodes[0], 1)

brunoerg commented at 1:33 PM on June 7, 2022:

Did you duplicate this self.generate(self.nodes[0], 1), any reason? You moved it to here but you also added it in L684.

akankshakashyap commented at 2:14 PM on June 7, 2022:

Yes, generate mines a block so it has been duplicated to confirm the transactions preceding it.

brunoerg commented at 2:50 PM on June 8, 2022:

It's only a suggestion, but I think this way would fit better.

diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
index 338c31eea..71fda6362 100755
--- a/test/functional/rpc_fundrawtransaction.py
+++ b/test/functional/rpc_fundrawtransaction.py
@@ -103,13 +103,7 @@ class RawTransactionsTest(BitcoinTestFramework):
         self.generate(self.nodes[2], 1)
         self.generate(self.nodes[0], 121)
 
-        # coins for simple tests
-        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.5)
-        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.0)
-        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 5.0)
-
-        self.generate(self.nodes[0], 1)
-
+        self.test_watchonly()
         self.test_change_position()
         self.test_simple()
         self.test_simple_two_outputs()
@@ -132,7 +126,6 @@ class RawTransactionsTest(BitcoinTestFramework):
         self.test_many_inputs_fee()
         self.test_many_inputs_send()
         self.test_op_return()
-        self.test_watchonly()
         self.test_all_watched_funds()
         self.test_option_feerate()
         self.test_address_reuse()
@@ -681,6 +674,11 @@ class RawTransactionsTest(BitcoinTestFramework):
         self.nodes[0].lockunspent(False, [{"txid": self.watchonly_txid, "vout": self.watchonly_vout}])
 
         self.nodes[0].sendtoaddress(self.nodes[3].get_wallet_rpc(self.default_wallet_name).getnewaddress(), self.watchonly_amount / 10)
+                # coins for simple tests
+        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.5)
+        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.0)
+        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 5.0)
+
         self.generate(self.nodes[0], 1)
 
         inputs = []

DrahtBot commented at 1:15 AM on June 8, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

maflcko commented at 9:48 AM on June 15, 2022: member

Looks like the test fails:

https://cirrus-ci.com/task/5430192075177984?logs=ci#L3248

 node0 2022-06-07T11:27:41.734273Z [httpworker.2] [wallet/scriptpubkeyman.h:247] [WalletLogPrintf] [default wallet] keypool keep 36 
 node0 2022-06-07T11:27:41.734327Z [httpworker.2] [wallet/wallet.h:795] [WalletLogPrintf] [default wallet] Fee Calculation: Fee:166 Bytes:166 Tgt:0 (requested 0) Reason:"PayTxFee set" Decay 0.00000: Estimation: (-1 - -1) 0.00% 0.0/(0.0 0 mem 0.0 out) Fail: (-1 - -1) 0.00% 0.0/(0.0 0 mem 0.0 out) 
 node0 2022-06-07T11:27:41.734346Z [httpworker.2] [wallet/wallet.h:795] [WalletLogPrintf] [default wallet] Fee non-grouped = 166, grouped = 166, using grouped 
 node0 2022-06-07T11:27:41.734397Z [httpworker.2] [wallet/wallet.h:795] [WalletLogPrintf] [default wallet] CommitTransaction: 
                                   CTransaction(hash=11555aeee3, ver=2, vin.size=1, vout.size=2, nLockTime=42)
                                       CTxIn(COutPoint(fa7b6ef92c, 0), scriptSig=160014d09cac8bd4236f5e4c, nSequence=4294967294)
                                       CScriptWitness(30440220022222b2401a269c4bb9728d33970cc2be9c3732cc40631d998c4a2ac57b90050220672604ebc282e8eaa8f38ef1e757a71b41c6074e87df803d5fb16e36fbd1dd9601, 03ecf6e8a92e3458899e1d03654f038f3651eeb35918b62d8bfc5e164f0a82f729)
                                       CTxOut(nValue=46.79999472, scriptPubKey=a91439bf4a58a40a2c02337843a515)
                                       CTxOut(nValue=1.10000000, scriptPubKey=a914c071d03bf92e1ed60a7ed31728)
 node0 2022-06-07T11:27:41.734467Z [httpworker.2] [wallet/wallet.h:795] [WalletLogPrintf] [default wallet] AddToWallet 11555aeee336a68ae3b9e3e702bc1f1e1b0f8da7ea3afc88a0c28a426f1379f3  newupdate 
 node0 2022-06-07T11:27:41.739028Z [httpworker.2] [wallet/wallet.h:795] [WalletLogPrintf] [default wallet] Submitting wtx 11555aeee336a68ae3b9e3e702bc1f1e1b0f8da7ea3afc88a0c28a426f1379f3 to mempool for relay 
 node0 2022-06-07T11:27:41.739523Z [httpworker.2] [txmempool.cpp:721] [check] [mempool] Checking mempool with 3 transactions and 3 inputs 
 node0 2022-06-07T11:27:41.739663Z [httpworker.2] [validationinterface.cpp:211] [TransactionAddedToMempool] [validation] Enqueuing TransactionAddedToMempool: txid=11555aeee336a68ae3b9e3e702bc1f1e1b0f8da7ea3afc88a0c28a426f1379f3 wtxid=beeef7d25f5dff35f739857b7850302fe8cb82c986ed81b4009d4c49c903a983 
 node0 2022-06-07T11:27:41.739698Z [httpworker.2] [txmempool.cpp:721] [check] [mempool] Checking mempool with 4 transactions and 4 inputs 
 node0 2022-06-07T11:27:41.739768Z [scheduler] [validationinterface.cpp:211] [operator()] [validation] TransactionAddedToMempool: txid=11555aeee336a68ae3b9e3e702bc1f1e1b0f8da7ea3afc88a0c28a426f1379f3 wtxid=beeef7d25f5dff35f739857b7850302fe8cb82c986ed81b4009d4c49c903a983 
 node0 2022-06-07T11:27:41.740066Z [scheduler] [wallet/wallet.h:795] [WalletLogPrintf] [default wallet] AddToWallet 11555aeee336a68ae3b9e3e702bc1f1e1b0f8da7ea3afc88a0c28a426f1379f3 
 node0 2022-06-07T11:27:41.740600Z [http] [httpserver.cpp:241] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:40714 
 node0 2022-06-07T11:27:41.740671Z [httpworker.3] [rpc/request.cpp:179] [parse] [rpc] ThreadRPCServer method=getmempoolentry user=__cookie__ 
 test  2022-06-07T11:27:41.741000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
                                       self.run_test()
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_fundrawtransaction.py", line 129, in run_test
                                       self.test_fee_4of5()
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_fundrawtransaction.py", line 495, in test_fee_4of5
                                       assert feeDelta >= 0 and feeDelta <= self.fee_tolerance
                                   AssertionError

akankshakashyap commented at 12:21 PM on June 15, 2022: contributor

Looks like the test fails:

The failing test is testing wether fundrawtransaction selects enough fees to cover the transaction after signing. Changes made in this PR should at most change the available coins inside the wallet.

I can't think of any reason for this test failing, other than there being some edge case in the fundrawtransaction's fees mechanism that these changes have chanced upon.

DrahtBot added the label Needs rebase on Jun 16, 2022

Remove test_simple_two_coins

The current test_simple_two_coins and test_simple are identical and
redundant. The test_simple_two_coins was changed in pr#6417 and asserts
ensuring the use of two input coins were removed.

df786b99e0

use changepos to get the change output

The current method assumes that a coin with value > 2 BTC was used to
fund the transaction, and therefore change will be greater than 1 BTC.
This can fail.

changepos allows us to get the change output much more reliably and
cleanly.

3e5bc2c6c4

akankshakashyap force-pushed on Jun 18, 2022

akankshakashyap commented at 8:50 PM on June 18, 2022: contributor

rebased.

akankshakashyap force-pushed on Jun 18, 2022

DrahtBot removed the label Needs rebase on Jun 18, 2022

maflcko commented at 6:21 AM on June 20, 2022: member

I don't know either why it happens. Though, this can not be merged with a failing test.

akankshakashyap force-pushed on Jun 20, 2022

akankshakashyap commented at 6:49 PM on June 20, 2022: contributor

The failing test passed after removing the 3rd commit.

maflcko commented at 7:25 AM on June 21, 2022: member

You'll have to adjust the pull request description?

achow101 commented at 7:29 PM on October 12, 2022: member

Closing this as it has not had a lot of conceptual support. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Oct 12, 2022

maflcko commented at 8:04 AM on October 13, 2022: member

The changes look correct to me

bitcoin locked this on Oct 13, 2023