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

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

Conversation

@ebfull
Copy link
Contributor

@ebfull ebfull commented Oct 29, 2016

Closes #1705.

The transaction selection logic in miner.cpp was not updated to account for JoinSplit value. This caused issues that include, but are not limited to, miners not including pure JoinSplit transactions in their blocks.

@daira
Copy link
Contributor

@daira daira commented Oct 29, 2016

ACK code, but needs tests. (Wow, that simple?)

@bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Oct 29, 2016

on mobile: what are the other issues mentioned above? How does nfeedelta impact tx selection? Tx were included in testnet and regtest mode, but there aren't significantly more tx now that a miner picks from, right?

@daira
Copy link
Contributor

@daira daira commented Oct 30, 2016

I don't understand why miners shouldn't include absolutely all transactions until they hit the block limit, or some other limit that is a well-defined standard rule. In fact I thought we'd explicitly decided to turn off dust checks!

Copy link
Contributor

@daira daira left a comment

Does this fix the whole problem? A transaction with only a JoinSplit and zero public input (possibly also zero public output) should still be promptly mined.

The assumption that it's reasonable to prioritise transactions by their publicly visible value is fundamentally broken for Zcash; we simply don't have enough information to do that. I thought that's why we decided to disable the "dust" provisions entirely. It's not clear to me whether the problem is that we didn't succeed in fully disabling them, or whether there is another layer of (de)prioritisation that we missed.

In any case, the purpose of fees is to prevent denial-of-service attacks and incentivize inclusion of transactions by miners who are following their own, non-default, policy. They're not there because of some ideological commitment to node costs needing to be paid for by this specific mechanism. As long as transaction volumes are low enough not to result in full blocks and the mining rewards are high, there really isn't any problem with the default client including as many transactions as it can. We should fix the immediate problem by doing that, rather than assuming that the transaction inclusion policy arrived at by Bitcoin 0.11.2 is suitable for Zcash.

@ebfull
Copy link
Contributor Author

@ebfull ebfull commented Oct 30, 2016

Do we want to go for a more aggressive change (removing the priority logic) for a 1.0.1 fix of this sort?

@str4d str4d modified the milestone: 1.0.1 Oct 31, 2016
@buzztiaan
Copy link

@buzztiaan buzztiaan commented Oct 31, 2016

@daira , are you saying no-fee transactions should work by design?

@bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Oct 31, 2016

ACK and tested empirically.

@@ -9,6 +9,7 @@ zcash_gtest_SOURCES = \
gtest/test_checktransaction.cpp \
gtest/json_test_vectors.cpp \
gtest/json_test_vectors.h \
gtest/test_fillnewblock.cpp \

This comment has been minimized.

@str4d

str4d Oct 31, 2016
Contributor

This test sets Params(), and IIRC there's an ordering thing that needs to be correct. @bitcartel?

This comment has been minimized.

@bitcartel

bitcartel Oct 31, 2016
Contributor

Yes, test_checktransaction has to run before the tests that set Params. Since you're updating the file worth putting a comment next to test_checktransaction to remind us not to move it from the top.

@str4d str4d assigned ebfull and unassigned str4d Nov 1, 2016
@ebfull
Copy link
Contributor Author

@ebfull ebfull commented Nov 1, 2016

I will write tests for this on my flight!

@ebfull ebfull force-pushed the ebfull:valid-fee-selection branch from 2d1ecee to 2b2bc69 Nov 1, 2016
@ebfull
Copy link
Contributor Author

@ebfull ebfull commented Nov 1, 2016

What happened to the commits that @str4d pushed here? I needed those :(

@str4d
Copy link
Contributor

@str4d str4d commented Nov 1, 2016

@ebfull did you force-push the first commit again? I have them locally, I can push them on top again.

@str4d
Copy link
Contributor

@str4d str4d commented Nov 1, 2016

@ebfull pushed to your branch 😃

@ebfull
Copy link
Contributor Author

@ebfull ebfull commented Nov 1, 2016

Thanks, got em.

🛫

@ebfull ebfull force-pushed the ebfull:valid-fee-selection branch from 2d1ecee to 2b2bc69 Nov 2, 2016
@nathan-at-least
Copy link
Contributor

@nathan-at-least nathan-at-least commented Nov 2, 2016

@zkbot r-

Sorry for the confusion. I said "ack" on slack to mean "I see a request to do a review, so I'll do a review now".

@nathan-at-least
Copy link
Contributor

@nathan-at-least nathan-at-least commented Nov 2, 2016

How has this been tested? How do I recreate the test? Who has tested it?

@nathan-at-least
Copy link
Contributor

@nathan-at-least nathan-at-least commented Nov 2, 2016

ACK on (~15 min) code review.

@nathan-at-least
Copy link
Contributor

@nathan-at-least nathan-at-least commented Nov 2, 2016

It looks like the refactor to do dependency injection is too burdensome now. Can we create automated tests in the ./qa/pull-tester/ framework which fail without this patch, but succeed with the patch?

@ebfull
Copy link
Contributor Author

@ebfull ebfull commented Nov 2, 2016

Okay, now there's an RPC test for this which fails if the patch is not applied.

self.nodes[0].generate(1)
for x in range(0,50):
self.nodes[0].sendtoaddress(addrtest, 0.01);

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?

This comment has been minimized.

@ebfull

ebfull Nov 2, 2016
Author 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.

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.

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?

This comment has been minimized.

@ebfull

ebfull Nov 2, 2016
Author 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.

@str4d
Copy link
Contributor

@str4d str4d commented Nov 2, 2016

utACK on the tests.

@bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Nov 2, 2016

See new comment above about number of txs in mempool --> https://github.com/zcash/zcash/pull/1718/files/52676958d15987019b4c4b22f4340a33690bc772#r86243821.

Does not block. ACK.

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.

Copy link
Contributor

@nathan-at-least nathan-at-least left a comment

Until I understand why these new loops exercise the buggy code, and that this test is not regtest-specific I can't yet ack.

self.nodes[0].generate(1)
for x in range(0,50):
self.nodes[0].sendtoaddress(addrtest, 0.01);

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.

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

@ebfull
Copy link
Contributor Author

@ebfull ebfull commented Nov 2, 2016

I stated in another comment that the bug this PR is trying to fix is only triggered when the mempool contains other transactions. The reason why our existing RPC tests didn't catch the bug (that pure joinsplits were being mined at all in our tests) is that the mempool had no other objects in it.

@bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Nov 2, 2016

One klunky way to create a negative test would be to use a runtime parameter and pass it into a new qa test (like in wallet_protectcoinbase.py):

if (!mapArgs.count("-fudgeparam")) {
  theonelinefix
}
@nathan-at-least
Copy link
Contributor

@nathan-at-least nathan-at-least commented Nov 3, 2016

Ok, I change my block to an ACK (based only on code-review / questions).

I'm going to test it locally with w/out the fix to convince myself, but this doesn't block the merge or release.

@nathan-at-least
Copy link
Contributor

@nathan-at-least nathan-at-least commented Nov 3, 2016

Ok, I've locally tested to produce a proper fail (w/out the fix) and a proper success (w/ the fix).

ACK source review and testing.

@zkbot r+

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Nov 3, 2016

📌 Commit 5267695 has been approved by nathan-at-least

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Nov 3, 2016

Testing commit 5267695 with merge 1430573...

zkbot pushed a commit that referenced this pull request Nov 3, 2016
Properly account for JoinSplit value when deciding if a transaction should be placed in a mined block.

Closes #1705.

The transaction selection logic in miner.cpp was not updated to account for JoinSplit value. This caused issues that include, but are not limited to, miners not including pure JoinSplit transactions in their blocks.
@daira
Copy link
Contributor

@daira daira commented Nov 3, 2016

I remain deeply skeptical that this is an adequate fix; actually I'm sure that it isn't. If I understand correctly, after this PR it sums the public inputs to JoinSplits as well as the public transparent inputs as one of the inputs to the decision of which transactions to include in a block. So if the public inputs are zero, then the PR will have no effect, and the bug will still be present. That can't be right; there is absolutely no reason for a transaction with zero JoinSplit inputs to be deprioritized.

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Nov 3, 2016

💔 Test failed - zcash

@str4d
Copy link
Contributor

@str4d str4d commented Nov 3, 2016

@daira the problem is that for transactions containing pure JoinSplits and no transparent inputs, the fee is paid using vpub_new, which is a transparent input to the value pool that was not being accounted for.

@bitcartel
Copy link
Contributor

@bitcartel bitcartel commented Nov 3, 2016

Move gtest/test_checkblock.cpp to the end of the list of tests in Makefile.gtest.include. Fix the whitespace/tab issue on the line gtest/json_test_vectors.h. Locally build and test zcash-gtest. It should no longer fail. (Same issue with #1735 for miner metrics, so perhaps make a PR to fix the zcash-gtest issue and then rebase both on master)

@ebfull
Copy link
Contributor Author

@ebfull ebfull commented Nov 3, 2016

@zkbot retry

@zkbot
Copy link
Collaborator

@zkbot zkbot commented Nov 3, 2016

Testing commit 5267695 with merge 9eb852e...

zkbot pushed a commit that referenced this pull request Nov 3, 2016
Properly account for JoinSplit value when deciding if a transaction should be placed in a mined block.

Closes #1705.

The transaction selection logic in miner.cpp was not updated to account for JoinSplit value. This caused issues that include, but are not limited to, miners not including pure JoinSplit transactions in their blocks.
@zkbot
Copy link
Collaborator

@zkbot zkbot commented Nov 3, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 5267695 into zcash:master Nov 3, 2016
1 check passed
1 check passed
homu Test successful
Details
@daira
Copy link
Contributor

@daira daira commented Nov 3, 2016

Oh, GetJoinSplitValueIn sums the outputs of JoinSplits. That function name makes no sense to me (it should be something like GetTotalInputToTransparentPoolFromJoinSplits), but I see how the fix works now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants