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

Properly account for JoinSplit value when deciding if a transaction should be placed in a mined block. #1718

Merged
merged 2 commits into from Nov 3, 2016
Jump to file or symbol
Failed to load files and symbols.
+8 −0
Diff settings

Always

Just for now

Viewing a subset of changes. View all
Prev

Test that a pure joinsplit will mine if other transactions are in the…

… mempool.
  • Loading branch information...
ebfull committed Nov 2, 2016
commit 52676958d15987019b4c4b22f4340a33690bc772
@@ -36,6 +36,14 @@ def run_test(self):
receive_result = self.nodes[0].zcrawreceive(zcsecretkey, joinsplit_result["encryptednote1"])
assert_equal(receive_result["exists"], True)
# The pure joinsplit we create should be mined in the next block
# despite other transactions being in the mempool.
addrtest = self.nodes[0].getnewaddress()
for xx in range(0,10):
self.nodes[0].generate(1)
for x in range(0,50):
self.nodes[0].sendtoaddress(addrtest, 0.01);

This comment has been minimized.

@bitcartel

bitcartel Nov 2, 2016

Contributor

I debugged the mempool at this point and, as expected, there were 50 txs due to the last iteration of the outer loop.. The mining of 10 blocks seems unnecessary. You could just have the 'for x' loop to create transactions, whilst dropping the 'for xx' outer loop.

@bitcartel

bitcartel Nov 2, 2016

Contributor

I debugged the mempool at this point and, as expected, there were 50 txs due to the last iteration of the outer loop.. The mining of 10 blocks seems unnecessary. You could just have the 'for x' loop to create transactions, whilst dropping the 'for xx' outer loop.

joinsplit_tx = self.nodes[0].createrawtransaction([], {})
joinsplit_result = self.nodes[0].zcrawjoinsplit(joinsplit_tx, {receive_result["note"] : zcsecretkey}, {zcaddress: 39.8}, 0, 0.1)

This comment has been minimized.

@bitcartel

bitcartel Nov 2, 2016

Contributor

How about adding a call to check that the txid of the joinsplit tx is in the mempool, and then after the block is mined, check that the txid is no longer in the mempool and that it has 1 confirmation?

@bitcartel

bitcartel Nov 2, 2016

Contributor

How about adding a call to check that the txid of the joinsplit tx is in the mempool, and then after the block is mined, check that the txid is no longer in the mempool and that it has 1 confirmation?

This comment has been minimized.

@ebfull

ebfull Nov 2, 2016

Contributor

We do this below, sort of. exists becomes True when the transaction finds its way into a block.

@ebfull

ebfull Nov 2, 2016

Contributor

We do this below, sort of. exists becomes True when the transaction finds its way into a block.

This comment has been minimized.

@str4d

str4d Nov 2, 2016

Contributor

I'm okay with this being indirect for now.

@str4d

str4d Nov 2, 2016

Contributor

I'm okay with this being indirect for now.

This comment has been minimized.

@bitcartel

bitcartel Nov 2, 2016

Contributor

I know but the zcraw calls are deprecated and at some point will be removed entirely. Not a blocker for this PR though.

@bitcartel

bitcartel Nov 2, 2016

Contributor

I know but the zcraw calls are deprecated and at some point will be removed entirely. Not a blocker for this PR though.

This comment has been minimized.

@nathan-at-least

nathan-at-least Nov 2, 2016

Contributor

Didn't this test already pass (with or without the new nested for-loops)? Doesn't receive_result['exists'] == True`` before and after this change?

I don't understand how the presence of non-JS-only transactions matters, so I don't understand why these loops target the code in question.

Finally, how do I know this test isn't testing a behavior that's unique to regtest mode whereas mainnet will behave differently wrt fee calculations, priority, and transaction selection?

@nathan-at-least

nathan-at-least Nov 2, 2016

Contributor

Didn't this test already pass (with or without the new nested for-loops)? Doesn't receive_result['exists'] == True`` before and after this change?

I don't understand how the presence of non-JS-only transactions matters, so I don't understand why these loops target the code in question.

Finally, how do I know this test isn't testing a behavior that's unique to regtest mode whereas mainnet will behave differently wrt fee calculations, priority, and transaction selection?

This comment has been minimized.

@ebfull

ebfull Nov 2, 2016

Contributor

This patch is blocking the release of 1.0.1.

The change that I made allows the test that I wrote to pass. It does not pass without my patch, which is what you wanted.

The reason why it behaves like this is that the bug has a side-effect which is only noticeable when the mempool has other stuff in it. The exact conditions are on line 266 in miner.cpp. "Free" transactions are skipped and not entered into a block. Our existing tests would have caught the bug that this PR is fixing if we were doing other stuff with the mempool as well. So I've modified the test to stress that as well.

@ebfull

ebfull Nov 2, 2016

Contributor

This patch is blocking the release of 1.0.1.

The change that I made allows the test that I wrote to pass. It does not pass without my patch, which is what you wanted.

The reason why it behaves like this is that the bug has a side-effect which is only noticeable when the mempool has other stuff in it. The exact conditions are on line 266 in miner.cpp. "Free" transactions are skipped and not entered into a block. Our existing tests would have caught the bug that this PR is fixing if we were doing other stuff with the mempool as well. So I've modified the test to stress that as well.

ProTip! Use n and p to navigate between commits in a pull request.