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

mempool/blockchain: Preserve seqlock behavior. #1570

Merged
merged 2 commits into from Jan 24, 2019

Conversation

Projects
None yet
6 participants
@davecgh
Copy link
Member

davecgh commented Jan 18, 2019

The changes to reverse the utxoset semantics also had the side effect of correcting the behavior for sequence locks when inside of a block to the expected behavior instead of the actual current consensus behavior.

In order to avoid that, this modifies the blockchain consensus rules to properly preserve the old incorrect behavior of sequence locks within blocks and also updates the mempool to to reject transactions accordingly thus providing parity between the two.

The behavior needs to be corrected, but since it constitutes a change to the consensus rules, a vote will be necessary.

A second commit adds tests to ensure the incorrect sequence lock behavior of the current consensus rules is preserved.

In addition, in order to allow similar style tests in the future, this adds a function named quickVoteActivationParams which provides a unique set of chain parameters which allow the tests to more quickly activate an agenda while still creating a fully valid test chain that consists of full blocks.

@jrick

jrick approved these changes Jan 18, 2019

Copy link
Member

jrick left a comment

Definitely the right approach to fixing this.

@dajohi

dajohi approved these changes Jan 18, 2019

Copy link
Member

dajohi left a comment

testnet miner tok

@xaur

This comment has been minimized.

Copy link

xaur commented Jan 21, 2019

If current consensus rules have anything like version, it can be used in names e.g. V5Semantics orpreV6Semantics instead of oldSemantics because after a few more forks it will be unclear what is "old".

Show resolved Hide resolved blockchain/sequencelock.go Outdated

@davecgh davecgh force-pushed the davecgh:multi_preserve_pre_14_seqlock_behavior branch from 8c303d3 to c070d14 Jan 22, 2019

@davecgh

This comment has been minimized.

Copy link
Member Author

davecgh commented Jan 22, 2019

I've updated this to include a full set of tests to ensure the proper legacy semantics are preserved as well as updated the implementation accordingly.

@davecgh

This comment has been minimized.

Copy link
Member Author

davecgh commented Jan 22, 2019

@xaur That's a fair point, however, in this particular case, the code is only temporary and can entirely be removed after a vote which corrects the behavior is performed. It isn't always possible to remove legacy code, but it is in this case since the intended rules are more permissive.

@matheusd
Copy link
Member

matheusd left a comment

My only question is whether it would be useful to add explicit tests for block disapproval cases.

They end up resolving into either the b4..b6 or b6..b9 case, depending on whether or not the disapproved block has a tx spending from b0.Transactions[0], so not entirely sure if it would require new test cases for that.

Show resolved Hide resolved blockchain/validate_test.go Outdated

@davecgh davecgh force-pushed the davecgh:multi_preserve_pre_14_seqlock_behavior branch from c070d14 to 69e02d0 Jan 22, 2019

Show resolved Hide resolved blockchain/validate_test.go Outdated

@davecgh davecgh force-pushed the davecgh:multi_preserve_pre_14_seqlock_behavior branch from 69e02d0 to 9624137 Jan 22, 2019

@davecgh

This comment has been minimized.

Copy link
Member Author

davecgh commented Jan 22, 2019

Updated to include an explicit test for block disapproval case.

@davecgh davecgh force-pushed the davecgh:multi_preserve_pre_14_seqlock_behavior branch from 9624137 to 0bba84a Jan 22, 2019

@matheusd
Copy link
Member

matheusd left a comment

Couldn't think of any more tests to add now.

Tested on mainnet by stopping before block 310728 and trying to submit block 00000000000000001905d9167008829dc055b7320808a788549ea6cc3f8092df and tx
495559c93da67b88b3ad92d8ecc4ec54df47fa441fdc56dc4f21728ee2782fe7 which end up blocked due to triggering the error.

@jrick

jrick approved these changes Jan 23, 2019

davecgh added some commits Jan 18, 2019

mempool/blockchain: Preserve seqlock behavior.
The changes to reverse the utxoset semantics also had the side effect of
correcting the behavior for sequence locks when inside of a block to the
expected behavior instead of the actual current consensus behavior.

In order to avoid that, this modifies the blockchain consensus rules to
properly preserve the old incorrect behavior of sequence locks within
blocks and also updates the mempool to to reject transactions
accordingly thus providing parity between the two.

The behavior needs to be corrected, but since it constitutes a change to
the consensus rules, a vote will be necessary.
blockchain: Add tests for legacy seqlock behavior.
This adds tests to ensure the incorrect sequence lock behavior of the
current consensus rules is preserved.

In addition, in order to allow similar style tests in the future, this
adds a function named quickVoteActivationParams which provides a unique
set of chain parameters which allow the tests to more quickly activate
an agenda while still creating a fully valid test chain that consists of
full blocks.

@davecgh davecgh force-pushed the davecgh:multi_preserve_pre_14_seqlock_behavior branch from 0bba84a to 6088b18 Jan 24, 2019

@davecgh davecgh merged commit 6088b18 into decred:master Jan 24, 2019

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davecgh davecgh deleted the davecgh:multi_preserve_pre_14_seqlock_behavior branch Jan 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment