Workaround for Hardhat last block timestamp bug #644
Conversation
|
Would it be possible to keep the local clock offset implementation when not testing with hardhat? Or does that lead to problems as well? |
There was a problem hiding this comment.
LGTM, did you report the Hardhat bug upstream? I would suggest to do so ;-)
Btw. just so you know, the clock is now used in other places in Codex as well (for example in the validation of Storage Request when calling POST API call for creating one). If the clock gets synced out a lot (eq. for some reason no block arriving) it could lead to potentially unexpected behavior. But IMHO, it should not really happen.
|
I've re-added the clock offset, and the only way to know if it works will be in the distributed tests unfortunately. I will build a new image from this branch and see if the only Marketplace test we have in the dist tests works. |
emizzle
force-pushed
the
fix/sales/hardhat-onchain-clock
branch
2 times, most recently
from
03d182a to
4f37d86
Compare
|
I've run the MarketplaceExample distributed test and it expectedly fails, but that is not a problem introduced with the changes in this PR. I think that we are likely safe with these changes, though it is difficult to tell without more robust marketplace tests in the dist test suite. As we add more tests, if this becomes an issue, we will at least be aware of it. |
emizzle
force-pushed
the
fix/sales/hardhat-onchain-clock
branch
from
4f37d86 to
34e0af7
Compare
Likely due to a Hardhat bug in which the callbacks for subscription events are called and awaited before updating its local understanding of the last block time, Hardhat will report a block time in the `newHeads` event that is generally 1 second before the time reported from `getLatestBlock.timestamp`. This was causing issues with the OnChainClock's offset and therefore the `now()` used by the `OnChainClock` would sometimes be off by a second (or more), causing tests to fail. This commit introduce a `codex_use_hardhat` compilation flag, that when set, will always get the latest block timestamp from Hardhat via the `getLatestBlock.timestamp` RPC call for `OnChainClock.now` calls. Otherwise, the last block timestamp reported in the `newHeads` event will be used. Update the docker dist tests compilation flag for simulated proof failures (it was not correct), and explicitly add the `codex_use_hardhat=false` for clarity.
increases pointer by 67 blocks each period increase
emizzle
force-pushed
the
fix/sales/hardhat-onchain-clock
branch
from
87c68a3 to
d0c5912
Compare
Likely due to a Hardhat bug in which the callbacks for subscription events are called and awaited before updating its local understanding of the last block time, Hardhat will report a block time in the
newHeadsevent that is generally 1 second before the time reported fromgetLatestBlock.timestamp. This was causing issues with the OnChainClock's offset and therefore thenow()used by theOnChainClockwould sometimes be off by a second (or more), causing tests to fail.This commit introduce a
codex_use_hardhatcompilation flag, that when set, will always get the latest block timestamp from Hardhat via thegetLatestBlock.timestampRPC call forOnChainClock.nowcalls. Otherwise, the last block timestamp reported in thenewHeadsevent will be used.Update the docker dist tests compilation flag for simulated proof failures (it was not correct), and explicitly add the
codex_use_hardhat=falsefor clarity.This is a PR that is a smaller chunk of #607 to attempt to make it smaller and more digestible.
Note
Bumps codex-contracts-eth to codex-storage/codex-contracts-eth#75, which likely should be merged before merging this PR.