Modify proposervm window #1638
Modify proposervm window #1638
Conversation
Now blocks can only be stale when new Block is added.
There was a problem hiding this comment.
Now that the window is only modified in Add I think we can get away with using a read lock in Oldest and Length.
There was a problem hiding this comment.
removed lock for the two functions
utils/window/window_test.go
Outdated
| // Now we're one second past the ttl of 10 seconds of when [4] was added, | ||
| // so all existing elements should be evicted. | ||
| clock.Set(epochStart.Add(22 * time.Second)) |
There was a problem hiding this comment.
This statement doesn't seem to do anything anymore - should we remove it?
StephenButtolph
added
enhancement
|
If we are reopening this code, would it make sense to even keep the time window around if we are capping it at I don't have a "problem" with this, just thinking about how the shortened window could impact stability (how many surges of 64 blocks are there on the P-Chain?). (this is not necessarily a reflection on this PR but more of a discussion of the original issue) |
Co-authored-by: Stephen Buttolph <stephen@avalabs.org> Signed-off-by: nathan haim <nathan.haim@free.fr>
| w.lock.Lock() | ||
| defer w.lock.Unlock() |
There was a problem hiding this comment.
Can we grab the read lock here
There was a problem hiding this comment.
The function does not modify the content. The lock was previously used because Oldest and Length were potentially removing content. It was to prevent concurrent update.
There was a problem hiding this comment.
Though Length and Oldest don't update any values it's possible that another thread will call Add which will result in Length and Oldest having race-y reads
There was a problem hiding this comment.
A change has been pushed 👍 Sorry I thought that this kind of concurrent write/read in our use-case was meaningless.
I think the question here is actually more: "how frequently does one node end up accepting a block more than 64 blocks ahead of another node" which is essentially never based on our metrics. Also remember we don't end up setting the P-chain height to the |
|
Thanks a lot for the change @najeal 🙏 |
Why this should be merged
It resolves the issue #1292
How this works
It replaces the current tip of the window (wall clock time) by a new one: the timestamp of the newest block added in the window.
It reduces the time range of the window from 5 minutes to 2 minutes. (Need to be validated because the issue was not proposing new value).
How this was tested
It was required to change unit tests of the window.