Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[test] Remove -txindex to start nodes #15404

Merged
merged 1 commit into from Feb 19, 2019

Conversation

Projects
None yet
7 participants
@amitiuttarwar
Copy link
Contributor

commented Feb 14, 2019

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

@fanquake fanquake added the Tests label Feb 14, 2019

@@ -91,25 +90,32 @@ def run_test(self):

txid = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 0.1)
self.sync_all()

This comment has been minimized.

Copy link
@amitiuttarwar

amitiuttarwar Feb 14, 2019

Author Contributor

@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.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 19, 2019

Member

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)

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

This comment has been minimized.

Copy link
@amitiuttarwar

amitiuttarwar Feb 14, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 19, 2019

Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK 31e7cad

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

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:remove_txindex branch 3 times, most recently from 8933a67 to 207588b Feb 16, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

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.

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:remove_txindex branch from 207588b to 8e4b4f6 Feb 18, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

re-utACK 8e4b4f6

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

utACK 8e4b4f6

@laanwj laanwj merged commit 8e4b4f6 into bitcoin:master Feb 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 19, 2019

Merge #15404: [test] Remove -txindex to start nodes
8e4b4f6 Address test todos by removing -txindex to nodes. Originally added when updating getrawtransaction to stop searching unspent utxos. (Amiti Uttarwar)

Pull request description:

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

Tree-SHA512: d080953c3b0d2e5dca2265a15966dc25985a614c9cc86271ecd6276178ce428c85e262c24df92501695c32fed7beec0339b989f03cce91b57fb2efba201b7809
@jnewbery
Copy link
Member

left a comment

utACK 8e4b4f6

Nice cleanup. Thanks @amitiuttarwar!

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 19, 2019

Member

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.

"""
Return index to output of txid with value amount
Raises exception if there is none.
"""
txdata = node.getrawtransaction(txid, 1)
txdata = node.getrawtransaction(txid, 1, blockhash)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 19, 2019

Member

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.

@@ -91,25 +90,32 @@ def run_test(self):

txid = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 0.1)
self.sync_all()

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 19, 2019

Member

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)

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 19, 2019

Member

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])
@@ -38,31 +38,29 @@ def find_spendable_utxo(node, min_value):

raise AssertionError("Unspent output equal or higher than %s not found" % min_value)

txs_mined = {} # txindex from txid to blockhash

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 19, 2019

Member

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

@amitiuttarwar amitiuttarwar deleted the amitiuttarwar:remove_txindex branch Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.