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
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -20,7 +20,8 @@ Supported API

Given a transaction hash: returns a transaction in binary, hex-encoded binary, or JSON formats.

For full TX query capability, one must enable the transaction index via "txindex=1" command line / configuration option.
By default, this endpoint will only search the mempool.
To query for a confirmed transaction, enable the transaction index via "txindex=1" command line / configuration option.

#### Blocks
`GET /rest/block/<BLOCK-HASH>.<bin|hex|json>`
@@ -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.


class SegWitTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 3
# This test tests SegWit both pre and post-activation, so use the normal BIP9 activation.
# TODO: remove -txindex. Currently required for getrawtransaction call.
self.extra_args = [
[
"-rpcserialversion=0",
"-vbparams=segwit:0:999999999999",
"-addresstype=legacy",
"-txindex"
],
[
"-blockversion=4",
"-rpcserialversion=1",
"-vbparams=segwit:0:999999999999",
"-addresstype=legacy",
"-txindex"
],
[
"-blockversion=536870915",
"-vbparams=segwit:0:999999999999",
"-addresstype=legacy",
"-txindex"
],
]

@@ -157,10 +155,10 @@ def run_test(self):

self.log.info("Verify previous witness txs skipped for mining can now be mined")
assert_equal(len(self.nodes[2].getrawmempool()), 4)
block = self.nodes[2].generate(1) # block 432 (first block with new rules; 432 = 144 * 3)
blockhash = self.nodes[2].generate(1)[0] # block 432 (first block with new rules; 432 = 144 * 3)
sync_blocks(self.nodes)
assert_equal(len(self.nodes[2].getrawmempool()), 0)
segwit_tx_list = self.nodes[2].getblock(block[0])["tx"]
segwit_tx_list = self.nodes[2].getblock(blockhash)["tx"]
assert_equal(len(segwit_tx_list), 5)

self.log.info("Verify default node can't accept txs with missing witness")
@@ -174,15 +172,16 @@ def run_test(self):
self.fail_accept(self.nodes[0], "mandatory-script-verify-flag", p2sh_ids[NODE_0][WIT_V1][0], False, witness_script(True, self.pubkey[0]))

self.log.info("Verify block and transaction serialization rpcs return differing serializations depending on rpc serialization flag")
assert(self.nodes[2].getblock(block[0], False) != self.nodes[0].getblock(block[0], False))
assert(self.nodes[1].getblock(block[0], False) == self.nodes[2].getblock(block[0], False))
for i in range(len(segwit_tx_list)):
tx = FromHex(CTransaction(), self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
assert(self.nodes[2].getrawtransaction(segwit_tx_list[i]) != self.nodes[0].getrawtransaction(segwit_tx_list[i]))
assert(self.nodes[1].getrawtransaction(segwit_tx_list[i], 0) == self.nodes[2].getrawtransaction(segwit_tx_list[i]))
assert(self.nodes[0].getrawtransaction(segwit_tx_list[i]) != self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
assert(self.nodes[1].getrawtransaction(segwit_tx_list[i]) == self.nodes[2].gettransaction(segwit_tx_list[i])["hex"])
assert(self.nodes[0].getrawtransaction(segwit_tx_list[i]) == bytes_to_hex_str(tx.serialize_without_witness()))
assert(self.nodes[2].getblock(blockhash, False) != self.nodes[0].getblock(blockhash, False))
assert(self.nodes[1].getblock(blockhash, False) == self.nodes[2].getblock(blockhash, False))

for tx_id in segwit_tx_list:
tx = FromHex(CTransaction(), self.nodes[2].gettransaction(tx_id)["hex"])
assert(self.nodes[2].getrawtransaction(tx_id, False, blockhash) != self.nodes[0].getrawtransaction(tx_id, False, blockhash))
assert(self.nodes[1].getrawtransaction(tx_id, False, blockhash) == self.nodes[2].getrawtransaction(tx_id, False, blockhash))
assert(self.nodes[0].getrawtransaction(tx_id, False, blockhash) != self.nodes[2].gettransaction(tx_id)["hex"])
assert(self.nodes[1].getrawtransaction(tx_id, False, blockhash) == self.nodes[2].gettransaction(tx_id)["hex"])
assert(self.nodes[0].getrawtransaction(tx_id, False, blockhash) == bytes_to_hex_str(tx.serialize_without_witness()))

self.log.info("Verify witness txs without witness data are invalid after the fork")
self.fail_accept(self.nodes[2], 'non-mandatory-script-verify-flag (Witness program hash mismatch) (code 64)', wit_ids[NODE_2][WIT_V0][2], sign=False)
@@ -538,7 +537,7 @@ def mine_and_test_listunspent(self, script_list, ismine):
tx.rehash()
signresults = self.nodes[0].signrawtransactionwithwallet(bytes_to_hex_str(tx.serialize_without_witness()))['hex']
txid = self.nodes[0].sendrawtransaction(signresults, True)
self.nodes[0].generate(1)
txs_mined[txid] = self.nodes[0].generate(1)[0]
sync_blocks(self.nodes)
watchcount = 0
spendcount = 0
@@ -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])
f = BytesIO(hex_str_to_bytes(txraw))
txtmp.deserialize(f)
for j in range(len(txtmp.vout)):
@@ -43,8 +43,7 @@ class RESTTest (BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 2
# TODO: remove -txindex. Currently required for getrawtransaction call.
self.extra_args = [["-rest", "-txindex"], []]
self.extra_args = [["-rest"], []]

def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
@@ -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)

self.nodes[1].generatetoaddress(1, not_related_address)
self.sync_all()
bb_hash = self.nodes[0].getbestblockhash()

assert_equal(self.nodes[1].getbalance(), Decimal("0.1"))

self.log.info("Load the transaction using the /tx URI")
self.log.info("Test the /tx URI")

json_obj = self.test_rest_request("/tx/{}".format(txid))
assert_equal(json_obj['txid'], txid)

# Check hex format response
hex_response = self.test_rest_request("/tx/{}".format(txid), req_type=ReqType.HEX, ret_type=RetType.OBJ)
assert_greater_than_or_equal(int(hex_response.getheader('content-length')),
json_obj['size']*2)

spent = (json_obj['vin'][0]['txid'], json_obj['vin'][0]['vout']) # get the vin to later check for utxo (should be spent by then)
# get n of 0.1 outpoint
n, = filter_output_indices_by_value(json_obj['vout'], Decimal('0.1'))
spending = (txid, n)

self.log.info("Query an unspent TXO using the /getutxos URI")

json_obj = self.test_rest_request("/getutxos/{}-{}".format(*spending))
self.nodes[1].generatetoaddress(1, not_related_address)
self.sync_all()
bb_hash = self.nodes[0].getbestblockhash()

assert_equal(self.nodes[1].getbalance(), Decimal("0.1"))

# Check chainTip response
json_obj = self.test_rest_request("/getutxos/{}-{}".format(*spending))
assert_equal(json_obj['chaintipHash'], bb_hash)

# Make sure there is one utxo
@@ -274,17 +280,6 @@ def run_test(self):
json_obj = self.test_rest_request("/headers/5/{}".format(bb_hash))
assert_equal(len(json_obj), 5) # now we should have 5 header objects

self.log.info("Test the /tx URI")

tx_hash = block_json_obj['tx'][0]['txid']
json_obj = self.test_rest_request("/tx/{}".format(tx_hash))
assert_equal(json_obj['txid'], tx_hash)

# Check hex format response
hex_response = self.test_rest_request("/tx/{}".format(tx_hash), req_type=ReqType.HEX, ret_type=RetType.OBJ)
assert_greater_than_or_equal(int(hex_response.getheader('content-length')),
json_obj['size']*2)

self.log.info("Test tx inclusion in the /mempool and /block URIs")

# Make 3 tx and mine them on node 1
@@ -20,8 +20,6 @@ class PSBTTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = False
self.num_nodes = 3
# TODO: remove -txindex. Currently required for getrawtransaction call.
self.extra_args = [["-txindex"], ["-txindex"], ["-txindex"]]

def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
@@ -161,11 +159,11 @@ def run_test(self):
node1_addr = self.nodes[1].getnewaddress()
node2_addr = self.nodes[2].getnewaddress()
txid1 = self.nodes[0].sendtoaddress(node1_addr, 13)
txid2 =self.nodes[0].sendtoaddress(node2_addr, 13)
self.nodes[0].generate(6)
txid2 = self.nodes[0].sendtoaddress(node2_addr, 13)
blockhash = self.nodes[0].generate(6)[0]
self.sync_all()
vout1 = find_output(self.nodes[1], txid1, 13)
vout2 = find_output(self.nodes[2], txid2, 13)
vout1 = find_output(self.nodes[1], txid1, 13, blockhash=blockhash)
vout2 = find_output(self.nodes[2], txid2, 13, blockhash=blockhash)

# Create a psbt spending outputs from nodes 1 and 2
psbt_orig = self.nodes[0].createpsbt([{"txid":txid1, "vout":vout1}, {"txid":txid2, "vout":vout2}], {self.nodes[0].getnewaddress():25.999})
@@ -344,9 +342,9 @@ def run_test(self):
addr = self.nodes[1].getnewaddress("", "p2sh-segwit")
txid = self.nodes[0].sendtoaddress(addr, 7)
addrinfo = self.nodes[1].getaddressinfo(addr)
self.nodes[0].generate(6)
blockhash = self.nodes[0].generate(6)[0]
self.sync_all()
vout = find_output(self.nodes[0], txid, 7)
vout = find_output(self.nodes[0], txid, 7, blockhash=blockhash)
psbt = self.nodes[1].createpsbt([{"txid":txid, "vout":vout}], {self.nodes[0].getnewaddress("", "p2sh-segwit"):Decimal('6.999')})
analyzed = self.nodes[0].analyzepsbt(psbt)
assert not analyzed['inputs'][0]['has_utxo'] and not analyzed['inputs'][0]['is_final'] and analyzed['inputs'][0]['next'] == 'updater' and analyzed['next'] == 'updater'
@@ -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.

self.extra_args = [["-addresstype=legacy", "-txindex"], ["-addresstype=legacy", "-txindex"], ["-addresstype=legacy", "-txindex"]]

def skip_test_if_missing_module(self):
@@ -410,12 +410,12 @@ def sync_mempools(rpc_connections, *, wait=1, timeout=60, flush_scheduler=True):
# Transaction/Block functions
#############################

def find_output(node, txid, amount):
def find_output(node, txid, amount, *, blockhash=None):
"""
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.

for i in range(len(txdata["vout"])):
if txdata["vout"][i]["value"] == amount:
return i
@@ -26,8 +26,7 @@
class AbandonConflictTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 2
# TODO: remove -txindex. Currently required for getrawtransaction call.
self.extra_args = [["-minrelaytxfee=0.00001", "-txindex"], []]
self.extra_args = [["-minrelaytxfee=0.00001"], []]

def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
@@ -24,8 +24,6 @@ class WalletTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 4
self.setup_clean_chain = True
# TODO: remove -txindex. Currently required for getrawtransaction call.
self.extra_args = [[], [], ["-txindex"], []]

def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.