[test] Remove -txindex to start nodes #15404

amitiuttarwar commented at 5:18 AM on February 14, 2019: contributor

Original todos added when removing getrawtransaction default behavior of searching unspent utxos.

fanquake added the label Tests on Feb 14, 2019

in test/functional/interface_rest.py:92 in 31e7cad0bd outdated

  89 | @@ -91,25 +90,32 @@ def run_test(self):
  90 |  
  91 |          txid = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 0.1)
  92 |          self.sync_all()

amitiuttarwar commented at 5:45 AM on February 14, 2019:

@jnewbery, mostly an FYI, but feedback welcome.. as per our conversation I've been trying to improve the variable propagation delay. The solution Ive been looking at is to use the [create / sign / send]rawtransaction RPCs to directly broadcast the tx to both nodes. My attempt lead me running into the absurdly-high-fee error from sendrawtransaction. And the simple fix of passing in the “allow” boolean leads to an insufficient-funds error when trying to sendtoaddress later. I could continue the bandaids by adding more funds, but instead.. I decided to just put up these changes since they don’t degrade the performance & I’ll probably keep digging because I’m curious to understand where fee is getting set & what Im doing wrong.

jnewbery commented at 4:15 PM on February 19, 2019:

These changes are good on their own and achieve the goal of removing -txindex.

To manually propagate the transaction, leave the sendtoaddress() in place, but then fetch the hex transaction using getrawtransaction(txid) (which works since the tx is still in the mempool), and then submit that transaction to the other nodes using sendrawtransaction(hex_tx)

in test/functional/rpc_rawtransaction.py:45 in 31e7cad0bd outdated

  41 | @@ -42,7 +42,6 @@ class RawTransactionsTest(BitcoinTestFramework):
  42 |      def set_test_params(self):
  43 |          self.setup_clean_chain = True
  44 |          self.num_nodes = 3
  45 | -        # TODO: remove -txindex. Currently required for getrawtransaction call.

amitiuttarwar commented at 5:46 AM on February 14, 2019:

I removed this todo from the code, but its still on my personal list :) I am interested in improving this test so it actually covers the unique intended functionalities. Eg. since we are currently passing -txindex to every node, this test wouldn’t catch if getrawtransaction using just the blockhash started failing. I’ve started to dive in and I’m headed down the path of using -txindex on some (not all) of the nodes to make the tests more robust. Does this seem reasonable?

jnewbery commented at 3:40 PM on February 19, 2019:

I’m headed down the path of using -txindex on some (not all) of the nodes

This seems like a very good idea to me.

amitiuttarwar commented at 6:06 AM on February 14, 2019: contributor

slightly tangential, but since I'm in the vicinity... am I missing a reason for why the getblocktemplatecall is made twice with the same params & same assertions are checked? https://github.com/bitcoin/bitcoin/pull/15404/files?utf8=%E2%9C%93&diff=unified#diff-99b0f22f7fc18956271d518d2c5e3816R101

DrahtBot commented at 8:31 AM on February 14, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14053 (Add address-based index (attempt 4?) by marcinja)

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.

MarcoFalke commented at 4:36 PM on February 14, 2019: member

utACK 31e7cad0bd80aaae8ffa22090387c9ce55746872

MarcoFalke added this to the milestone 0.18.0 on Feb 14, 2019

in test/functional/rpc_psbt.py:164 in 31e7cad0bd outdated

 163 | +        txid2 = self.nodes[0].sendtoaddress(node2_addr, 13)
 164 | +        blockhash = self.nodes[0].generate(6)[0]
 165 |          self.sync_all()
 166 | -        vout1 = find_output(self.nodes[1], txid1, 13)
 167 | -        vout2 = find_output(self.nodes[2], txid2, 13)
 168 | +        vout1 = find_output(self.nodes[1], txid1, 13, blockhash = blockhash)

practicalswift commented at 5:30 PM on February 15, 2019:

Nit: Drop spaces around = in named argument (blockhash).

in test/functional/rpc_psbt.py:165 in 31e7cad0bd outdated

 164 | +        blockhash = self.nodes[0].generate(6)[0]
 165 |          self.sync_all()
 166 | -        vout1 = find_output(self.nodes[1], txid1, 13)
 167 | -        vout2 = find_output(self.nodes[2], txid2, 13)
 168 | +        vout1 = find_output(self.nodes[1], txid1, 13, blockhash = blockhash)
 169 | +        vout2 = find_output(self.nodes[2], txid2, 13, blockhash = blockhash)

practicalswift commented at 5:30 PM on February 15, 2019:

Same here.

amitiuttarwar commented at 7:57 PM on February 16, 2019:

how do you catch these? are you running a linter on just the changed code?

MarcoFalke commented at 3:48 PM on February 18, 2019:

You are free to use a formatter on the diff, see https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md#writing-code. However, we do not enforce a specific style, because developers use different editors and commit workflows. @practicalswift I think that purely stylistic feedback about code is not useful (especially when the pull request has already been reviewed and would thus require a re-review). Though, it is fine to leave stylistic feedback as part of a feedback that has actual concerns about the code.

amitiuttarwar commented at 7:40 PM on February 18, 2019:

ah, missed that, thanks.

hm, I hear your point about stylistic feedback & reviews, but I think the feedback should be directed to me instead of practicalswift. If other reviewers made comments that caused me to make deeper code changes, I could incorporate the whitespace updates. But not worth re-committing for just that. ✔️Noted.

In this case changes to master required an update to rpc_psbt anyways.

amitiuttarwar force-pushed on Feb 16, 2019

laanwj commented at 1:05 AM on February 17, 2019: member

Concept ACK.

This breaks the psbt test at the moment, at least when merged to master:

rpc_psbt.py                           | ✖ Failed  | 19 s

Address test todos by removing -txindex to nodes.

Originally added when updating getrawtransaction to stop searching unspent utxos.

8e4b4f683a

amitiuttarwar force-pushed on Feb 18, 2019

MarcoFalke commented at 3:50 PM on February 18, 2019: member

re-utACK 8e4b4f683a0b342cec24cd51b1e98433034ea2ea

laanwj commented at 3:31 PM on February 19, 2019: member

utACK 8e4b4f683a0b342cec24cd51b1e98433034ea2ea

laanwj merged this on Feb 19, 2019

laanwj closed this on Feb 19, 2019

laanwj referenced this in commit 38429c4b62 on Feb 19, 2019

in test/functional/test_framework/util.py:418 in 8e4b4f683a

 415 |      """
 416 |      Return index to output of txid with value amount
 417 |      Raises exception if there is none.
 418 |      """
 419 | -    txdata = node.getrawtransaction(txid, 1)
 420 | +    txdata = node.getrawtransaction(txid, 1, blockhash)

jnewbery commented at 4:13 PM on February 19, 2019:

Note that this is a minor change in behaviour of the test framework. Previously, the JSON request would always have an array of two values for its args. Now it has an array of three args where the final arg may be null. That's ok since getrawtransaction() rpc will ignore a null argument as the third positional argument and so this doesn't change behaviour on the node.

in test/functional/feature_segwit.py:583 in 8e4b4f683a

 579 | @@ -581,7 +580,7 @@ def create_and_mine_tx_from_txids(self, txids, success=True):
 580 |          tx = CTransaction()
 581 |          for i in txids:
 582 |              txtmp = CTransaction()
 583 | -            txraw = self.nodes[0].getrawtransaction(i)
 584 | +            txraw = self.nodes[0].getrawtransaction(i, 0, txs_mined[i])

jnewbery commented at 4:36 PM on February 19, 2019:

nit: the second argument to getrawtransaction() is a bool, so this should be:

txraw = self.nodes[0].getrawtransaction(i, False, txs_mined[i])

Even better, use named args and drop the unused param to make it obvious what the params are for:

txraw = self.nodes[0].getrawtransaction(txid=i, blockhash=txs_mined[i])

in test/functional/feature_segwit.py:41 in 8e4b4f683a

  37 | @@ -38,31 +38,29 @@ def find_spendable_utxo(node, min_value):
  38 |  
  39 |      raise AssertionError("Unspent output equal or higher than %s not found" % min_value)
  40 |  
  41 | +txs_mined = {} # txindex from txid to blockhash

jnewbery commented at 4:37 PM on February 19, 2019:

micronit: tiny preference for this being a member variable in SegWitTest rather than a global.

jnewbery commented at 4:39 PM on February 19, 2019: member

utACK 8e4b4f683a0b342cec24cd51b1e98433034ea2ea

Nice cleanup. Thanks @amitiuttarwar!

amitiuttarwar deleted the branch on Feb 19, 2019

deadalnix referenced this in commit 345b8e5656 on May 16, 2020

Munkybooty referenced this in commit 81e9a6ef42 on Sep 6, 2021

Munkybooty referenced this in commit 45319d7d08 on Sep 7, 2021

Munkybooty referenced this in commit 30f85dfca0 on Sep 7, 2021

Munkybooty referenced this in commit aef1b122ba on Sep 7, 2021

Munkybooty referenced this in commit 98ebe2cf8b on Sep 9, 2021

Munkybooty referenced this in commit b09b6e7cb6 on Sep 10, 2021

Munkybooty referenced this in commit ed492846d1 on Sep 13, 2021

Munkybooty referenced this in commit 7006a38ba5 on Sep 13, 2021

DrahtBot locked this on Dec 16, 2021