fix: correctly handle null blocks when detecting an expensive fork #7210
Conversation
Stebalien
force-pushed
the
fix/fork-check
branch
from
310cfe0
to
4841b0a
Compare
Codecov Report
@@ Coverage Diff @@
## master #7210 +/- ##
==========================================
- Coverage 39.13% 39.12% -0.02%
==========================================
Files 608 608
Lines 64602 64620 +18
==========================================
Hits 25283 25283
- Misses 34930 34943 +13
- Partials 4389 4394 +5
Continue to review full report at Codecov.
|
Also improve/fix documentation to reflect the _actual_ tipset that's passed into upgrades. And update some comments. fixes #7192
Stebalien
force-pushed
the
fix/fork-check
branch
from
4841b0a
to
165735d
Compare
| @@ -99,7 +99,6 @@ func (sm *StateManager) ApplyBlocks(ctx context.Context, parentEpoch abi.ChainEp | |||
| } | |||
There was a problem hiding this comment.
nit: not at all your bug but reading the above code it looks like line 82's error message is wrong and confusing and makes sense to fix now.
There was a problem hiding this comment.
I'd rather not touch that for now. I don't think that error is consensus critical, but I'd rather not touch it.
| @@ -140,25 +149,33 @@ func (sm *StateManager) CallWithGas(ctx context.Context, msg *types.Message, pri | |||
| // run the fork logic in `sm.TipSetState`. We need the _current_ | |||
There was a problem hiding this comment.
Is there a good reason why Call happens against tipset parent state and CallWithGas happens against tipset state? As far as I understand is what requires the introduction of hasExpensiveForkBetween. Beyond removing extra work it would also just be nice to standardize these two similar calls
There was a problem hiding this comment.
CallWithGas applies the message on-top-of the tipset to provide better gas estimation, I think. By executing it on-top-of the last tipset, instead of on-top-of the parent tipset state, we get the best gas estimates.
I'd prefer to always use the parent state, but I'm not sure what might break. I'm especially concerned that, e.g., it might (rarely) cause issues with window post:
- See we're at height X.
- Try to push the proof message.
- Fail because the window isn't open yet.
chain/stmgr/forks.go
Outdated
| func (sm *StateManager) hasExpensiveFork(ctx context.Context, height abi.ChainEpoch) bool { | ||
| // Returns true if executing the current tipset would trigger an expensive fork. | ||
| // | ||
| // - If the tipset is the genesis, this function always returns false. |
There was a problem hiding this comment.
As written now this invariant depends on the sm.expensiveUpgrades table which in reasonable scenarios could potentially contain "expensive" upgrades if say we did all migrations at the genesis epoch.
It might be better to override the table to always return false on 0 to maintain this invariant. Any network doing this would probably be doing very little work in the expensive migration since nothing has happened in state yet.
Also all existing callers seem to implicitly assume that this is the case anyway by only checking heights > 0 so why not make it official.
There was a problem hiding this comment.
Because the function is called hasExpensiveFork, not hasExpensiveForkOrIsGenesis 😄. Basically, I don't want to surprise users.
At the moment, genesis matters to all callers because there's really no other option. But I'd like to keep that as an explicit decision.
There was a problem hiding this comment.
But I've fixed the issue in Call where I wasn't handling this consistently.
chain/stmgr/forks_test.go
Outdated
| @@ -292,14 +307,20 @@ func TestForkRefuseCall(t *testing.T) { | |||
| GasFeeCap: types.NewInt(0), | |||
| } | |||
|
|
|||
| nullStart := abi.ChainEpoch(testForkHeight - nullsBefore) | |||
| nullLength := abi.ChainEpoch(nullsBefore + nullsAfter) | |||
|
|
|||
| for i := 0; i < 50; i++ { | |||
There was a problem hiding this comment.
Not your bug: I don't love that testForkHeight is a global and that this test relies on knowing that its value is 40 in order to actually test anything. Maybe make testForkHeight not a global, or change 50 to something like 2 * testForkHeight?
There was a problem hiding this comment.
Well, being a global is intentional. But yeah, 50 being hard-coded is a problem.
Otherwise, we could try to lookup the parent of the block at height 0.
|
Lint fails |
Also improve/fix documentation to reflect the actual tipset that's passed into upgrades.
And update some comments.
fixes #7192