Prevote nil if proposal is not timely#7415
Prevote nil if proposal is not timely#7415ancazamfir merged 28 commits intowb/proposer-based-timestampsfrom
Conversation
williambanfield
left a comment
There was a problem hiding this comment.
This is definitely the right direction for this code change. The final piece of this is adding tests using the pbtsTestHarness.
These tests will test delivery of various blocks at height 2. The test harness creates one state machine under test and a set of fake validators that create proposals and votes. At height 2, the state machine under test is not the proposer, so we use that height to verify its behavior when it receives a block at different times and with various timestamps.
The test harness allows us to set when the 1. What time the block will be delivered to the state machine, 2. what the timestamp of the block will be set to and 3. what the timing params will be set to for the state machine. The tests then collect the prevotes from the state machine under test and allows us to examine them.
We should be able to use that to construct tests for the various conditions of timely-ness and ensure that the state machine votes properly in each case.
|
Created: #7481 to track the failure in tests (01). That may be fixed in |
|
Hey Anca, nice job getting all of this working with the test. I mulled this over a bit. I think that the best way to perform the check would be to just save the timestamp at which we receive the proposal. It makes the check a bit more obvious to anyone reading the code. Here's a commit implementing this solution, let me know what you think. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
yes, I think this is better, thanks! |
|
I think this is looking good. The main change is to change the |
A few tests were failing. I think many use height |
Have these changes any impact elsewhere? I would consider opening an issue on that, if it is not "excessive caution". |
@cason: |
|
@ancazamfir Given that the genesis time change causes additional failures and complexity, I think we should split that change out into a different PR and just merge this change with the timely checks. |
a6e5a89 to
693bae7
Compare
Ok i reverted the changes. They can be worked on/ reviewed in #7541 |
* Prevote nil if not timely * William's suggestion to get the proposal from the proposer instead of generating it. * Don't check rhs for genesis block * Update IsTimely to match the specification * Fix proposal tests * Add more timely tests and check votes * Mark proposal invalid in SetProposal, fix in the future test * save proposal time on roundstate * received -> receive * always reset proposal time * Add IsTimely test for genesis proposal * Check timely before ValidateBlock * Review comments from Daniel Co-authored-by: William Banfield <wbanfield@gmail.com>
See #6942:
Update the proposal validation logic to prevote nil for proposals with times that fail the `timely` check if the proof of lock round is `-1`. If the proof of lock round is not -1, do not check timely.