Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement FendermintExterns::get_tipset_cid #508

Closed
aakoshh opened this issue Jan 3, 2024 · 3 comments · Fixed by #568
Closed

Implement FendermintExterns::get_tipset_cid #508

aakoshh opened this issue Jan 3, 2024 · 3 comments · Fixed by #568
Assignees
Labels
bug Something isn't working Fluence

Comments

@aakoshh
Copy link
Contributor

aakoshh commented Jan 3, 2024

Implement get_tipset_cid so any contract using the BLOCKHASH Solidity function doesn't crash the node.

Things to consider:

  • Operators can configure a limited size application history, after which not even CometBFT remembers block headers, so we can't look up block identity uniformly unless the limit is part of consensus.
  • Snapshots only export actor state, so after a snapshot restore we have no block history at all, or at least I don't think CometBFT promises that we do.
  • For these reasons I recommend that we add a new built-in actor with an AMT and schedule it to be executed by the cron actor and store a configurable (in genesis) number of block hashes in the actor state itself, so that snapshot export/import restores the pre-agreed length of history, and then we implement get_tipset_cid based on this actor.
@linear linear bot added Fluence bug Something isn't working labels Jan 4, 2024
@fridrik01 fridrik01 self-assigned this Jan 11, 2024
Copy link
Contributor

raulk commented Jan 12, 2024

For EVM compatibility, we only need a lookback of 256 blocks. It may be possible to ask CometBFT to sync X blocks from the network on node init. From then onwards, we assume CometBFT would maintain sync and inform us of new blocks so we can maintain the lookback live.

If it's possible to guarantee that CometBFT can sync N blocks, on init we could find a way to suspend Fendermint init until the lookback cache has been filled. However, @akosh.farkash pointed out that this may not be easy. It also couples IPC further to CometBFT, which ain't good.

As @akosh.farkash suggested, the alternative is to create a "chain history actor" to store the lookback in state. Resolving this syscall would then involve making a transient call to this actor.

Alternatively, that actor could be an extended version of the system actor; in which case we'd have to fork builtin-actors (bad), or replace the system actor with our custom version at genesis (better). I'd choose this path if we wanted to prevent system actor sprawl, but I also don't have strong opinions.

If we generalise this actor to something like a "chain metadata actor", it comes broader in scope and more useful down the line as a good home for future chain-dependent/specific functionality.

Copy link
Contributor

raulk commented Jan 12, 2024

In conclusion, I think @aakoshh has it right and this shouldn't be hard too implement. From my POV:

  • Create a new "chain metadata" actor the state data model Akosh suggested above. Take a look at the suggested interface below.
  • Bundle this actor in a separate bundle with its own manifest.
  • Load and link this actor at genesis, constructing it with a len determined by a code constant.
  • Figure out how to obtain the blockhash at the right time within the ABCI pipeline so we can feed it in (take into account the case where the contract calls BLOCKHASH 0, i.e. the current block, this happens in immediate execution chains like CometBFT/Ignite, unlike Filecoin)
  • Figure out if we need to fork cron, or if we should have a dedicated cron, or if we should call push_blockcid individually.
  • Implement the get_tipset_cid syscall by calling ChainMetadata::blockcid()

Chain metadata actor methods

struct State {
  blockcid_lookback: Amt<Optional<Cid>>,
}

struct ConstructorParams {
  blockcid_lookback_len: u64,
}

// method num = 1
pub fn constructor(params: ConstructorParams);

// method num = 2, this can take the epoch or can fetch it through a syscall; it's needed in case we need to skip positions? (e.g. null rounds)
pub fn push_blockcid(cid: Cid);

// method num = frc42!
// Returns the configured lookback length.
pub fn blockcid_lookback_len() -> u64;

// method num = frc42!
// Returns the CID of the block at [`rewind`] many blocks in the past.
// Can return None if the requested position was a null round in chains that support such behaviour.
// Returns an error if the requested position is beyond the lookback.
// Returns another error if the requested position is within lookback but would make us seek backwards beyond genesis.
pub fn blockcid(rewind: u64) -> Result<Optional<Cid>>;

@fridrik01 fridrik01 mentioned this issue Jan 16, 2024
12 tasks
@raulk raulk linked a pull request Jan 26, 2024 that will close this issue
12 tasks
@raulk
Copy link
Contributor

raulk commented Jan 26, 2024

Rest of the TODOs tracked in #568.

@raulk raulk closed this as completed Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Fluence
Projects
No open projects
Status: Triage
Development

Successfully merging a pull request may close this issue.

3 participants