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

multi: No stake height checks in check tx inputs. #1457

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Sep 14, 2018

This relies on #1456.

This removes a redundant check for validating that votes and revocations are not in a block before they are allowed from CheckTransactionInputs since it has nothing to do with validation of transaction inputs and it's an impossible condition to hit in the block validation path because the same check is done much earlier in the validation process.

It should also be noted that the checks this removes are also more permissive than they need to be because the first vote is not allowed until stake validation height, which is after stake enabled height. There is no effect on consensus because, as previously mentioned, the more restrictive checks earlier in the process prevent these checks from ever being exercised.

However, the mempool previously relied on this duplicate check to reject early votes and revocations, so this adds the checks, with the corrected heights, directly to the mempool early in the process where they more naturally belong. It is more efficient to reject the transactions in that scenario earlier before doing more work as well.

This is work towards #1145.

@davecgh davecgh added this to the 1.4.0 milestone Sep 14, 2018
@davecgh davecgh mentioned this pull request Sep 14, 2018
33 tasks
@davecgh
Copy link
Member Author

davecgh commented Sep 14, 2018

Test results from --simnet which has a StakeValidationHeight of 144:

2018-09-14 03:04:36.785 [INF] MINR: Block submitted via CPU miner accepted (hash 0055277f74c13adf5038e5a2d8b094ed0d2ad9e71e4e5f826fa3034ef43369f3, height 143, amount 346.53465346 DCR)
2018-09-14 03:04:36.959 [DBG] TXMP: Accepted vote cf512c66fe82098e37ba5bb591995948fe74c0e8346f0a479417d1ae1d44bee0 for block hash 0055277f74c13adf5038e5a2d8b094ed0d2ad9e71e4e5f826fa3034ef43369f3 (height 143), voting true on the transaction tree
2018-09-14 03:04:36.959 [DBG] TXMP: Accepted transaction cf512c66fe82098e37ba5bb591995948fe74c0e8346f0a479417d1ae1d44bee0 (pool size: 1)
2018-09-14 03:04:36.960 [DBG] TXMP: Accepted vote 945d57ae75dad40fe833a9e82b2ddf4c1b09142635bbfd9ee458fbfddb610506 for block hash 0055277f74c13adf5038e5a2d8b094ed0d2ad9e71e4e5f826fa3034ef43369f3 (height 143), voting true on the transaction tree
2018-09-14 03:04:36.960 [DBG] TXMP: Accepted transaction 945d57ae75dad40fe833a9e82b2ddf4c1b09142635bbfd9ee458fbfddb610506 (pool size: 2)
2018-09-14 03:04:36.961 [DBG] TXMP: Accepted vote ad71c9b80499052ebb60a35368c5bb317e05b41a1c76aeb61f077711c59e7e78 for block hash 0055277f74c13adf5038e5a2d8b094ed0d2ad9e71e4e5f826fa3034ef43369f3 (height 143), voting true on the transaction tree
2018-09-14 03:04:36.961 [DBG] TXMP: Accepted transaction ad71c9b80499052ebb60a35368c5bb317e05b41a1c76aeb61f077711c59e7e78 (pool size: 3)
2018-09-14 03:04:36.961 [DBG] TXMP: Accepted vote 8939719c2c97785f2b95600cfae54bf8f1890d6d5bd6c74a49b0c4096c60252c for block hash 0055277f74c13adf5038e5a2d8b094ed0d2ad9e71e4e5f826fa3034ef43369f3 (height 143), voting true on the transaction tree
2018-09-14 03:04:36.961 [DBG] TXMP: Accepted transaction 8939719c2c97785f2b95600cfae54bf8f1890d6d5bd6c74a49b0c4096c60252c (pool size: 4)
2018-09-14 03:04:36.962 [DBG] TXMP: Accepted vote 82c25c3012e0c12ca12061e77addbcd2f5afd4d1ce047a450e6cdecb6c972ba5 for block hash 0055277f74c13adf5038e5a2d8b094ed0d2ad9e71e4e5f826fa3034ef43369f3 (height 143), voting true on the transaction tree
2018-09-14 03:04:36.962 [DBG] TXMP: Accepted transaction 82c25c3012e0c12ca12061e77addbcd2f5afd4d1ce047a450e6cdecb6c972ba5 (pool size: 5)

@davecgh davecgh force-pushed the blockchain_no_stake_enable_height_checks_check_tx_inputs branch 2 times, most recently from 62de5d1 to 736ffc4 Compare September 14, 2018 09:54
@davecgh davecgh force-pushed the blockchain_no_stake_enable_height_checks_check_tx_inputs branch from 736ffc4 to 2c49d38 Compare September 14, 2018 16:22
This removes a redundant check for validating that votes and revocations
are not in a block before they are allowed from CheckTransactionInputs since
it has nothing to do with validation of transaction inputs and it's an
impossible condition to hit in the block validation path because the
same check is done much earlier in the validation process.

It should also be noted that the checks this removes are also more
permissive than they need to be because the first vote is not allowed
until stake validation height, which is after stake enabled height.
There is no effect on consensus because, as previously mentioned, the
more restrictive checks earlier in the process prevent these checks from
ever being exercised.

However, the mempool previously relied on this duplicate check to reject
early votes and revocations, so this adds the checks, with the corrected
heights, directly to the mempool early in the process where they more
naturally belong.  It is more efficient to reject the transactions in
that scenario earlier before doing more work as well.
@davecgh davecgh force-pushed the blockchain_no_stake_enable_height_checks_check_tx_inputs branch from 2c49d38 to b6c088d Compare September 14, 2018 18:28
@davecgh davecgh merged commit b6c088d into decred:master Sep 14, 2018
@davecgh davecgh deleted the blockchain_no_stake_enable_height_checks_check_tx_inputs branch September 14, 2018 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants