Delegcns

[aakoshh]

Summary of changes
Changes introduced in this pull request:

• Adds deleg_cns for Delegated Consensus which is a simplistic demo consensus where only a single miner can ever propose blocks.
• I moved some parts of state_api::miner_create_block to chain::persist_block_messages, namely the storing of messages as CID and the aggregation of BLS signatures. This looks like a common thing that will be required by everything that wants to produce a block, and the chain crate was already full of helper method to read things that were persisted this way. Only the BLS may be questionable there, but it seemed like a natural place for it to be in, and the crate already references the forest_crypto stuff.
• Added a select_messages_for_block method to the MessagePool to pick the messages which should go in the next block.

TODO:

• Generate a BLS key pair and set it as the worker and owner address of the nominated miner in a preseal.json file
• Create a template genesis.json file using forest genesis new-template CLI command and add the keys to it with forest genesis add-miner
• Find a way to turn the genesis.json template file into a genesis.car that daemon.rs expects to load
• Start a mining loop that produces a block at regular intervals with the right timestamps
• Select transactions from the mempool that can go into the block, observing the rules about fees and nonces
• Figure out how to add the miner private key to the wallet

Genesis file generation
It looks like I will have to rely on Lotus to generate the genesis file because:

• Forest has not implemented all the genesis commands, only new-template and add-miner
• whereas Eudico/Lotus have the genesis car command that produces all the MakeGenesisBlock function to put everything together. Eudico even has a subnet specific variant which seems to be a copy of the original with tweaks.
• The latest version of of the fvm_ipld_car library can only read CAR files, not write them.

The rabbit hole appears too deep for me to start implementing it, and I expect this might be on the roadmap of the Forest core team. Since all we need is a CAR file with the delegated miner address in it, using Lotus seems like an acceptable compromise until Forest has the capability to do this on its own.

Added a README which documents the process of producing a working genesis.car file.

The idea for starting nodes is that one of them will have a wallet that actually contains the private key of the miner, while the others don't. If the key of the chosen miner can be found in the wallet, then mining will commence, otherwise the node is treated as a delegator.

Other information and links

I'm not sure that the select_messages_for_block mechanism will work in the future as it only selects messages to be included in the next immediate block. It would not be suitable for building up a whole pipeline of unconfirmed messages, to be included in the next several blocks.

For example in the Tendermint example of a subnet consensus we can send transactions to Tendermint which comes up with a total ordering of them that is final, so a pre-requisite to work with Filecoin execution rules is for these to be ordered by the account sequence numbers, which the feeder is trying to do by inspecting the messages first. In theory we can have dozens of Tendermint blocks without creating Filecoin blocks from them immediately, as long as when we do, we put the messages in the same order. This would require a different method being available in the MessagePoolApi that can project the next messages indefinitely, maybe by getting a Map<Address, Sequence> to resume from, to which it can add anything new that would be suitable in a new block. It can still do the ordering, just chop off the prefix of certain Chains.

[adlrocha]

The more I think about it, the more I feel we'll need to decouple the use of miner actors for HC validators in non Filecoin EC consensus. Even if we want to still use the concepts of worker and owner addresses, it'd be good to decouple into a new actor without all the storage-specific functions.

For future iteration, it may be a good idea to just use account actors for validators in non-Filecoin EC consensus (I am aware this may require some changes to the validation logic, unfortunately).

[aakoshh]

You are right, the Genesis block will have lots of actors that have nothing to do with the consensus we run on a subnet. You did well in Eudico to take control there. Maybe when can talk to the Forest team before they replicate such functionality to do it in a more modular way. Tendermint and Cosmos provide a nice model here, but it was made to be that way. We can't rock the boat too much if we want to keep in sync with the core Forest changes.

[adlrocha]

We can't rock the boat too much if we want to keep in sync with the core Forest changes.

Definitely. I don't want to suffer again what we will potentially suffer when we push Eudico's changes to Lotus :) I feel that as the Forest team is smaller they will be happy with us contributing as long as we respect the compatibility with mainnet. I would definitely propose all the changes that we need (as long as they're non-breaking) and make Forest also "our thing". (Honestly, right now I think you are one of the top people with deep knowledge of the base code --which are few--)

[adlrocha]

I love this README, amazing job!! 🙏

[aakoshh]

Great! I added a version of the script that you can use as well, without Docker.

[adlrocha]

the variable name is very appropriate :)

[adlrocha]

LGTM. I love the Proposer approach for validators. I think it'll give us a lot of flexibility in the future. As you are using cfg to determine the type of consensus to spawn, we could maybe try to upstream this change also to Forest. WDYT?

[adlrocha]

Yes, this is the approach we follow in eudico. We expose the state manager to the mining process to access the required state.

[aakoshh]

Right, yes, I figured that Forest mimics Lotus in this regard.

It's just sort of an anti-pattern in Object Oriented programming to expose dependencies and chain together lots of accessors, like it.a().b().c().do_something(x) instead of it.do_abc(x), and manager.chain().database().put(...) completely obfuscates any ideas about what someone might want to do with manager, it's just a hold-all to allow you to do almost anything, because the ChainStore is super permissive.

Ideally we could give something like the MessagePool gets via the Provider interface, or limited views over individual stuff like ChainStoreReader. I left it like this because we're already passing the StateManager to the validate_block function, but it's worth a rethink at some point.

[adlrocha]

Just a quick note: there's no need for pre-sealed data to start delegated consensus (we don't need to on-board power to mine the chain).

[adlrocha]

I see, you need to have some pre-sealed data in advance because there's no command in lotus to add a miner in genesis without pre-sealed data (just realized while reading the script).

[adlrocha]

This is really helpful. We may find a slight blocker here, though, once we start spawning new subnets, as we won't be able to generate genesis files "on-the-fly" as we do in Eudico. We may need to make a slight de-tour to introduce the ability to write CAR files natively in Rust.

[aakoshh]

Yes, spinning up a subnet will be more of a manual process of aligning all the ducks than what it sounds like to be in Eudico. OTOH if we took a page out of the Cosmos handbook, it's only the subnet consensus code that really knows what it wants to put into its own Genesis parameters, so if we want to keep it flexible, it has to be like that to some extent.

That said I'd be happy to look into CAR generation to understand the formats better.

[adlrocha]

You may have come across this method already and it may not work as expected, but the analogous to this function is what we use in Eudico to import keys into the node's keystore/wallet: https://github.com/ChainSafe/forest/blob/5fc22340af3164d36bef40ecd71864c8c0b7bd10/forest/src/cli/wallet_cmd.rs#L126-L138

From reading the implementation it looks like it imports a single key in json format, not the whole keystore, but I may be wrong.

[aakoshh]

Thanks! I didn't look close enough and thought it might do the whole thing; hopefully this is it!

[adlrocha]

As discussed, we may need to decouple Filecoin EC block validation from the rest of the consensus protocols so we can use account actors directly instead of having to go through all of the identity dance of the miner actor.

[adlrocha]

Is this true? At least in the delegated consensus implementation of Eudico we propose a new block every epoch. There can't be "null" epochs.

[aakoshh]

It is true that the proposer doesn't leave empty epochs, however, the validation allows nulls, so it would not be against the rules if it did.

In any case it didn't seem like there was a distinction between epoch and height in Lotus. I'm more biased here by my notion of height in all other blockchains I have come across.

[adlrocha]

Why not? Do you think we should have a more restrictive API from the proposer?

[aakoshh]

Yes, I think we should only give it limited capabilities. The MessagePool has too many public mutable fields, so that was a non-starter to share (anyone with a direct reference to the message pool could insert/remove messages from its caches at will). The StateManager itself is not too bad, but the ChainStore itself has public methods like set_genesis and set_heaviest_tipset that we should not be able to call without going through the proper channels.

The blockstore is another one that would be best if it was passed directly, if access is needed. Currently you can reach it via ChainStore.db, ChainStore.blockstore(), ChainStore.blockstore_cloned(), StateManager.blockstore(), StateManger.blockstore_cloned(), StateManager.chain_store().blockstore(), etc.

The Provider that the MessagePool gets is better at limiting access to what the mempool can use, but in return exposed via the public api field, rather than kept for itself, so again it's not perfect. Also, at some point interfaces like this tend to become annoying as well: they collect all dependencies a component needs into one mockable package, but they also make a mess of individual concepts, like having a state manager, a chain store and a block store as things. It's tempting to put too much logic into the methods of the provider, rather than what it provides to.

It is a balancing act. Perhaps separating interfaces to a reader/writer part is a good start.

[adlrocha]

That's a great point. I think is definitely worth a refactor as we keep building the protocol.

[adlrocha]

The latest version of of the fvm_ipld_car library can only read CAR files, not write them.

Regarding this, we should maybe check with the FVM team if they are planning to implement CAR writing (it would make our lives significantly easier).

Something we'll also need to figure out is how to perform integration tests for HC and new consensus implementations, in Eudico we have itests:

• https://github.com/filecoin-project/eudico/blob/eudico/itests/eudico_consensus_test.go
• https://github.com/filecoin-project/eudico/blob/eudico/itests/eudico_hc_test.go
  But I couldn't find the equivalent in Forest. Both are concerns for our future selves, but just wanted to flag them in case you have any proposal. Thanks!

[aakoshh]

in Eudico we have itests:

Thanks for the links! Definitely these sort of tests that set up multiple nodes are where we want to be. I think the primary way of integration testing Forest at the moment is to try syncing with calibnet. For delegcns I didn't see that we'd gain huge benefits from automated tests in this PR, but it might be a good idea to follow up with a PR that where we set up some nodes in memory with real or mocked networking and let it run for a while. It may require some changes to the daemon.

Definitely more tests are needed when actual hierarchical consensus machinery is added to the mix, and when we implement new consensus protocols like Narwhal, since they won't have any reference like Lotus to be tested against.

[aakoshh]

As you are using cfg to determine the type of consensus to spawn, we could maybe try to upstream this change also to Forest. WDYT?

Agreed, it would be good to get their feedback on this approach! I wasn't too happy about the conditional setup be in the middle of the daemon code, but unfortunately I couldn't find a better tradeoff in the amount of boilerplate to look at when I tried to move those parts to be outside the main flow. At least it would not burden their baseline usecase with dynamic dispatch.

I will figure out the wallet and see if the thing starts up at all, then close this PR and open another one against the upstream branch (I can't find a way to switch the base of the PR back to the other repo, only the branch can be changed 😕 )

[aakoshh]

Closing as ChainSafe#1714 got merged.