fvm: return gasused when verifying consensus faults #235
Conversation
|
@Stebalien I assume |
rust/Cargo.toml
Outdated
| @@ -74,8 +74,8 @@ bindgen=[] | |||
| # Patched in | |||
| [patch.crates-io] | |||
| cid = { git = "https://github.com/multiformats/rust-cid", branch = "steb/cbor-hack" } | |||
| fvm = { git = "https://github.com/filecoin-project/ref-fvm", branch = "steb/extern" } | |||
| fvm_shared = { git = "https://github.com/filecoin-project/ref-fvm", branch = "steb/extern" } | |||
| fvm = { path = '../../../../fvm/fvm' } | |||
There was a problem hiding this comment.
Drop after filecoin-project/ref-fvm#330 lands, but not sure what to patch to? master? no patch at all?
There was a problem hiding this comment.
I think we can drop the patch entirely and just depend on the git version.
| @@ -69,6 +69,7 @@ func cgo_extern_verify_consensus_fault( | |||
| miner_id *C.uint64_t, | |||
| epoch *C.int64_t, | |||
| fault *C.int64_t, | |||
| gas_used *C.int64_t, | |||
There was a problem hiding this comment.
Given that we're already using an "out" pointer here, what if we passed in a pointer to some gas_remaining variable? That would let us stop early without changing the parameters. I.e.:
- Read the current gas remaining from the
gas_remainingparameter. - Update it locally, stopping if/when we run out.
- Write back the final value.
There was a problem hiding this comment.
Although I guess this might complicate some other parts of the API a bit? We can always revisit this later.
There was a problem hiding this comment.
Yeah, that's a fair point...I'm gonna open an issue and kick the can down the road for now
cgo/interface.go
Outdated
| @@ -30,7 +31,7 @@ const ( | |||
| type Externs interface { | |||
| GetChainRandomness(ctx context.Context, personalization crypto.DomainSeparationTag, epoch abi.ChainEpoch, entropy []byte) ([]byte, error) | |||
| GetBeaconRandomness(ctx context.Context, personalization crypto.DomainSeparationTag, epoch abi.ChainEpoch, entropy []byte) ([]byte, error) | |||
| VerifyConsensusFault(ctx context.Context, h1, h2, extra []byte) (*ConsensusFault, error) | |||
| VerifyConsensusFault(ctx context.Context, h1, h2, extra []byte) *ConsensusFaultWithGas | |||
There was a problem hiding this comment.
Maybe just return multiple values?
There was a problem hiding this comment.
(ConsensusFaultWithGas is a bit of a strange type)
rust/src/fvm/externs.rs
Outdated
| ) { | ||
| 0 => Ok(Some(ConsensusFault { | ||
| target: Address::new_id(miner_id), | ||
| 0 => Ok(ConsensusFaultWithGas { |
There was a problem hiding this comment.
Personally, I'd return a tuple instead of a new type.
Also, this was clearly broken before as we had no case that returned None, but I don't think that having a "no fault" variant is the right fix. See filecoin-project/ref-fvm#330 (comment).
cgo/extern.go
Outdated
| id, err := address.IDFromAddress(res.Target) | ||
| if err != nil { |
There was a problem hiding this comment.
Is this valid if there's "no fault"?
There was a problem hiding this comment.
Maybe we need to say:
- If there's "no fault", we write that back to
fault. - Otherwise, we write everything else.
Needs filecoin-project/ref-fvm#330