feat: reject V1P1 proofs on calibrationnet before cutoff #1755
Conversation
arajasek
force-pushed
the
asr/calibnet-hack
branch
from
125fd61 to
083f082
Compare
fvm/src/kernel/default.rs
Outdated
| if self.call_manager.context().epoch <= 498691 | ||
| && verify_info.proofs[0].post_proof == StackedDRGWindow32GiBV1P1 | ||
| { | ||
| let mut verify_info_copy = verify_info.clone(); |
There was a problem hiding this comment.
Other efforts to fail early, such as returning Err here, or panicking without calling verify_post lead to slightly different gas usage. This surprises me, since these operations seem to be uncharged, but 🤷
There was a problem hiding this comment.
Ah. Well, I think this returns Ok(false) on failure?
There was a problem hiding this comment.
I looked at the code and thoroughly convinced myself it did not, but...you're right. That's much nicer.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## release/v3.3 #1755 +/- ##
================================================
- Coverage 75.29% 73.87% -1.43%
================================================
Files 146 146
Lines 14124 14258 +134
================================================
- Hits 10635 10533 -102
- Misses 3489 3725 +236
|
fvm/src/kernel/default.rs
Outdated
| if self.call_manager.context().epoch <= 498691 | ||
| && verify_info.proofs[0].post_proof == StackedDRGWindow32GiBV1P1 | ||
| { | ||
| let mut verify_info_copy = verify_info.clone(); |
There was a problem hiding this comment.
Ah. Well, I think this returns Ok(false) on failure?
fvm/src/kernel/default.rs
Outdated
| let mut verify_info_copy = verify_info.clone(); | ||
| verify_info_copy.proofs[0].post_proof = StackedDRGWindow32GiBV1; | ||
| return t.record(catch_and_log_panic("verifying post", || { | ||
| verify_post(&verify_info_copy) |
There was a problem hiding this comment.
can we just replace verify_info and continue on to the verify_post call below? IMO, always cloning is fine. Otherwise, you should be able to write write something like:
let mut verify_info_tmp: WindowPoStVerifyInfo;
if ... {
verify_info_tmp = verify_info.clone();
verify_info = &verify_info_tmp;
}| #[allow(clippy::collapsible_if)] | ||
| if self.call_manager.context().network.chain_id == ChainID::from(314159) { | ||
| if self.call_manager.context().epoch <= 498691 | ||
| && verify_info.proofs[0].post_proof == StackedDRGWindow32GiBV1P1 |
There was a problem hiding this comment.
Can we double-check the index? Just in case?
fvm/src/kernel/default.rs
Outdated
| @@ -539,6 +541,19 @@ where | |||
| .call_manager | |||
| .charge_gas(self.call_manager.price_list().on_verify_post(verify_info))?; | |||
|
|
|||
| #[allow(clippy::collapsible_if)] | |||
| if self.call_manager.context().network.chain_id == ChainID::from(314159) { | |||
There was a problem hiding this comment.
Let's define a constant for the chain ID to make it clear where this applies.
arajasek
force-pushed
the
asr/calibnet-hack
branch
from
083f082 to
4e42e32
Compare
| let calibnet_chain_id = ChainID::from(314159); | ||
|
|
||
| #[allow(clippy::collapsible_if)] | ||
| if self.call_manager.context().network.chain_id == calibnet_chain_id { |
There was a problem hiding this comment.
Any reason not to collapse these checks? rustfmt should align everything nicely. Also, we can get a reference to the context first:
let ctx = self.call_manager.context();
if ctx.network.chain_id == calibnet_chain_id
&& ctx.epoch <= 498691
&& ... {There was a problem hiding this comment.
@Stebalien I wanted to explicitly have it wrapped in a "ARE YOU CALIBNET" top-level block. But we are biking all over this shed.
Agreed with getting context first.
There was a problem hiding this comment.
I don't really care. I just mildly prefer appeasing the linter rather than fighting it.
There was a problem hiding this comment.
But you're right, it makes sense to have a top-level check.
|
Unfortunately, switching to just returning I don't fully understand this, asking the Proofs folks for insight. Regardless, we can just go back to cloning and mutating the infos. |
arajasek
force-pushed
the
asr/calibnet-hack
branch
from
4e42e32 to
0f6f8af
Compare
|
What do you want to check? I thought you just meant that the index 0 exists (which I have done) |
This resolves the inconsistent state on calibrationnet that resulted from the bug fixed in #1750. It's unfortunate to have to have a patch like this, but we can drop it when we graduate to FVM4.