informalsystems / cometmock Goto Github PK

Problem statement:

It is annoying to manually cherry-pick all commits that should be backported to the primary branches v0.34.x and v0.37.x.
To avoid this, the backporting should happen automatically as much as possible.

Possible solutions:

• Backport via Github action?
• Mergify bot?

For now, I will try to solve this via Github action to avoid the reliance on another framework.

Is your feature request related to a problem? Please describe.
When an application crashes or is disconnected, CometMock should not crash.
It should also allow disconnected apps to reconnect, and new apps to connect during runtime
Optimally, validators that belong to apps which are currently disconnected have their signing_status set to false,
i.e. those validators do not sign. Similarly, CometMock should detect if two applications with the same private key are connected and produce double-sign evidence if that happens.

This feature allows test runs that kill and restart applications to run smoothly. It also seamlessly allows downtime the way tests already do it (although CometMock has its own native way to cause downtime as well).

Proposed solution

• Calls to the ABCI server are made with a timeout
• Create a new type that encapsulates the client and associates it to a validator
• When an app times out, the validator associated with that app is set to not sign
• Periodically retry apps
• When an app reconnects, start signing with its validator again

Additional context
The logic was started to be implemented in this branch main...ph/dockerfile, but there is an issue with the way the CometBFT client calls the ABCI server, see this Slack messahe or the code here - the problem is that the context is hardcoded to background.

The solution is to duplicate that code from CometBFT and weave the contexts through.

Sync logic

This issue needs to add in particular support for syncing apps that do not have the current height.

• Allow removal of nodes during runtime
• #70

Is your feature request related to a problem? Please describe.
CometMock needs to be updated to be compatible with CometBFT v0.37.
This has partially been done by replacing the cometbft libraries, but
compatibility with CosmosSDK v0.47 (which uses CometBFT v0.37) is not successful so far.

CometMock should be built on any commit to the primary branches,
which are v0.34.x, v0.37.x and main.

This allows identifying build errors without having to manually build on each commit.

Overview

Currently if you see the docker images being released

Release page here: https://github.com/informalsystems/CometMock/pkgs/container/cometmock/versions?filters%5Bversion_type%5D=tagged

Proposal

Ideally taged releases should be limited to:

• 0.x.x versions
• main, nightly

The automated tests for v0.34 are currently not running,
because we are missing a Docker image for a simapp compatible with v0.34.x Comet.
The problem appears to be that the image from https://hub.docker.com/layers/interchainio/simapp/v0.45.16/images/sha256-9254c29a2d0d30e77a8d3571691037fa067fe01a8d016f6abbeda966c820d560?context=explore is not available for the right platform (arm64). The Cosmos SDK itself only has releases for v0.47 in their repo.

The current tests work locally for me (make test-docker) but not in the CI.

Proposed solution:

Find (or create) another Docker image with a v0.34-compatible Cosmos-SDK app to use in the test here.

Details:

Failure logs from the CI can be found here https://github.com/informalsystems/CometMock/actions/runs/6405144331/job/17387099747
The branch in question is v0.34.x
Tests can be run locally via make test-docker
Changes likely need to be made here: https://github.com/informalsystems/CometMock/pull/67a

Is your feature request related to a problem? Please describe.
To interact with CometMock, we call its RPC endpoints.
From the command line, this can be done e.g. via curl and looks like this:

curl -H 'Content-Type: application/json' -H 'Accept:application/json' --data '{"jsonrpc":"2.0","method":"advance_time","params":{"duration_in_seconds": "36000000"},"id":1}' 127.0.0.1:22331

This looks a bit overbearing and puts a lot of burden and confusion on the end user.

Describe the solution you'd like
The cometmock binary should have subcommands that allow interacting with a running CometMock instance to
invoke all custom functionality.
This should look, for example, like

cometmock advance_time 3600000 --node tcp://127.0.0.1:22331

Describe alternatives you've considered
We could offer an alternative binary - it wouldn't even have to be written in Golang, since the functionality it would perform is miniscule (just putting together curl requests). However, I think the solution where there is just a single binary is preferable - it's how the CosmosSDK does it (running nodes but also doing one-shot interactions with the same binary).

Additional context
Add any other context or screenshots about the feature request here.

CometMock should provide a way to connect to new apps during runtime, e.g. to add validators.

Proposed Solution

We add a new CometMock-specific RPC endpoint in https://github.com/informalsystems/CometMock/blob/ph/v0.38.x-upgrade/cometmock/rpc_server/routes.go#L56:
register-node(address, node_home) which takes as arguments the listen-address of the new node and its home directory.

The node info needs to be added to abci_client.GlobalClient:

• A new AbciCounterpartyClient needs to be added to the Clients list
• It needs to get a signing status

Also, the application needs to be synced to the current height.
I think it is probably best to stop all block production, automatic or triggered by the user, until the application has been synced, since it gives the most control over when exactly the node joins.
This likely can utilize the blockMutex https://github.com/informalsystems/CometMock/blob/ph/v0.38.x-upgrade/cometmock/abci_client/client.go#L30

To sync, we need to load the information from the storage and essentially replay
the blocks seen so far. See the storage here: https://github.com/informalsystems/CometMock/blob/ph/v0.38.x-upgrade/cometmock/storage/storage.go#L61
This might require storing additional things in the storage, but I don't think so - it has the commits and blocks, which should suffice as far as I can tell.

The workflow to add a new node would then be the following:

• Start up the node
• Register the node with CometMock

Workaround

There is a workaround for the need to do this: Start all nodes during chain startup, even if they are not yet validators.
During runtime, all and only validator nodes will sign, so it suffices to make some nodes into validators during runtime purely on the app level.

Currently, I am trying to understand whether there are any big drawbacks to this workaround.
In the wild, people might want to connect new validators during runtime, so there might be a good reason to support this, but it is not clear how people want to test in practice.

Is your feature request related to a problem? Please describe.
When e.g. calling the status endpoint https://docs.cometbft.com/v0.34/rpc/#/Info/status,
currently CometMock just uses the first validator and prints information for it.
This means it is hard to actually get information for other validators.

Describe the solution you'd like
CometMock should offer a way to direct calls to other apps/make them look like they hit a specific validator.

One solution for this could be to offer several addresses that cometmock listens on as an rpc server.
Which address a call gets directed to determines which validator it corresponds to.
The addresses could be specified during startup.

Additional context
This issue is currently low priority, since there are workarounds for the use cases so far - e.g. for getting the node-id, the cosmos-sdk offers functionality to get it without having to go through the cometbft rpc server with

gaiad tendermint show-node-id

If the workarounds become too cumbersome for future issues, this should potentially be addressed.

Is your feature request related to a problem? Please describe.
When a new commit is pushed either to main or another branch,
a Docker image should be built and pushed to the Github container registry.
This should happen automatically via a GitHub action.
The images should be tagges by which branch they come from.
On non-main branches, likely only one image should be stored.
On main, several past images should be stored, to allow using a fixed version of CometMock.

The purpose of this is to remove the dependency on docker images that are pushed by hand.

Additional context
See how to work with the GitHub container registry here https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-container-registry

Is your feature request related to a problem? Please describe.
CometMock should be updated to align with CometBFT v0.38, to keep up to date with ICS when we update to Cosmos SDK v0.50.

While doing this, the steps taken should be documented to make the process easier in the future (create branch, add branch policy, update go.mod, ...).

TL;DR Summary

To allow more control over time, I propose introducing some flags:
--starting-timestamp to set the timestamp of the first block
(--starting-timestamp-from-genesis to take the timestamp from the Genesis file instead)
--block-time to determine how timestamps in blocks increase from one block to the next
--block-production-interval to determine how often CometMock produces blocks automatically
--auto-tx to determine whether CometMock produces a new block when a tx is broadcasted or not

Goal

The goal is to enable use cases where time needs to be controlled precisely, i.e.
where we control precisely the block timestamps.

This can be split into separate features:

Feature 1: Decoupling the initial timestamp from the system time

This feature is straightforward: There should be a way to set the initial timestamp completely independently from the system time.

This will be implemented by adding a new CLI flag, --starting-timestamp.
The default value is time.Now() at startup of CometMock.

Alternatively, one can also opt to use the Genesis time for the first block
with the --starting-timestamp-from-genesis flag.

Feature 2: Decoupling the time advancement from the system clock

This feature is more complex. I see that we want three modes for timestamps:
Timestamps increase at the same rate as the system clock
With this mode, the exact block timestamps depend on how fast blocks are processed by the app, networking delays, ...
This is the mode that is currently in place, and where the interval for blocks can be set by using --block-time, e.g. --block-time 5 makes it so that CometMock sleeps for 5 seconds in between building blocks, and bases the timestamp on the current time (shifted by any time advancements that were made).

Timestamps increase at a fixed rate
This is similar to the last mode, but now block timestamps do not depend on how fast blocks are processed by the app, but rather, block timestamps are always advanced by the fixed rate, no matter how much the system clock advances between blocks (but the user may shift timestamps by advancing time).

Timestamps only increase based on user inputs
This is a subcase of Timestamps increasing at a fixed rate, namely the case where the rate is 0.
Timestamps only increase when the user tells them to, and this (together with decoupling the initial time)
gives complete, precise control over the timestamps of blocks.

Feature 3: (Give an option to) Decouple block production from transaction broadcasting

Currently, CometMock immediately produces a new block when a transaction is broadcast.

This does not work well when more precise control over blocks is needed.
In particular, it does not work well with fixed-rate increasing timestamps because when many transactions are submitted (and each produces a new block), the block time would rapidly increase, which could e.g. make ibc connections go out of sync.

Implementation

I propose the addition of three new flags for the cometmock start command, and to change the block-time flag:

--auto-tx {true | false}

If true, when a tx is set to be broadcasted, a new block is immediately created which contains just this transaction.

If false, all transactions that are broadcasted are kept in a queue, and whenever a new block is created for any reason (either by explicitly calling the advance_blocks endpoint, or with the appropriate flags, by system time passing), all transactions in the queue are included in the block.

(There might be good reason to not include all transactions from the queue, but rather to only include transactions up to a certain byte limit).

Defaults to false.

auto-tx=true is CometMocks current behaviour.

--block-time {time_in_milliseconds}

This flag already exists, but I would propose to change its semantics. Currently, block-time governs how long CometMock sleeps before creating a new block, and the block timestamps are taken from local time.

I would instead change this as follows: When a new block is created, its timestamp is

new_block_timestamp = last_block_timestamp + time_advancement_since_last_block + block-time

where last_block_timestamp is the timestamp of the last block created by CometMock, time_advancement_since_last_block is the sum of time that CometMock was told to advance time by with calls to advance_time.

This means block-time would not govern how often blocks are produced, but only the timestamps of blocks.

If block-time is below 0, the system clock will be used as a reference for block timestamps, so the above equation would become

new_block_timestamp = system_time_when_block_is_created + time_advancements + starting_offset

where starting_offset is the offset as calculated from the starting_timestamp.

Defaults to 1 second.

Current behaviour is block-time=0.

--block-production-interval {time_in_milliseconds}

This flag will take on the previous role of the block-time flag.

Thus, it governs how often blocks are produced without user intervention. Precisely, it is how long CometMock sleeps after it finished automatically producing one block, before starting to produce the next. This depends on system time.

Setting this to -1 means blocks will not be produced automatically, and will only be produced when advance_blocks is called, or (when auto-tx is true) a tx is broadcasted.

Defaults to 1 second.

Current behaviour is that this flag can be set, but is currently called block-time.

User stories

1. Alice wants to have a test that is close to how a real, local testnet looks, but she wants to skip voting periods and be able to produce many empty blocks.

She can run with these settings:

cometmock [...] --block-time -1 --block-production-interval 1000 --auto-tx true

and her test run could look like this:

2. Bob wants a reliable way to test his application, which hashes something based on the block timestamps. He needs blocks to have the same sequence of timestamps every time he runs his tests. He doesn't mind which exact block transactions go into, he cares just that the block timestamps are right, and that some transactions can occur.

Bob can run with these settings

cometmock [...] --block-time 1000 --block-production-interval 1000 --auto-tx true --starting-timestamp 0

then Bobs test run could look like this:

3. Charlie wants to test some edge cases in the liquidation code of his lending smart contract. He needs to test what happens when a loan is liquidated precisely 11111 milliseconds after the loan was initiated, and afterwards in the same block someone else takes a loan.

He can run with settings like this:

cometmock [...] --block-time 0 --block-production-interval -1 --auto-tx false

--block-production-interval -1 meaning no blocks will be produced automatically.
--block-time 0 meaning timestamps will not increment automatically.

Then his test run can look like this:

CometMock, by default, produces one block per second.
However, we sometimes might want to control block production in a very granular manner,
e.g. to exactly reproduce some error scenarios where block numbers matter.

The planned solution to this is to add an optional argument that allows setting the block time, and in particular disabling automatic block production.

There should be documentation for the deficiencies where CometMock does not replicate the behaviour of CometBFT (yet).
For example, a folder with Markdown documentation.

CometMock needs to support equivocation aka double-signing.

Describe the solution you'd like
Steps for implementing this:

• Add a new cometmock-specific rpc endpoint to https://github.com/p-offtermatt/CometMock/blob/main/cometmock/rpc_server/routes.go
• Add logic for producing different types of evidence
• Add logic for propagating evidence to the application, see the byzantine_validators field in the BeginBlockRequest https://github.com/cometbft/cometbft/blob/0.35x/spec/abci/abci.md#beginblock

Additional context
Notes from a meeting with the CometBFT team on how to handle evidence

Reach out to the CometBFT team, in particular Jasmina and Sergio, for questions and pointers beyond the linked notes.

Currently, the automated build and test infrastructure for CometMock is very limited.
This allowed, for example, a commit that caused a build error to sneak by (see #14). In the interest of avoiding such errors, a few things should be done:

• #17
• #22
• #65
• #63
• #64

Is your feature request related to a problem? Please describe.
CometMock already supports causing DoubleSignEvidence.
There is, however, another type of evidence that we'd sometimes like to cause, namely
LightClientAttackEvidence, see https://github.com/tendermint/spec/blob/master/spec/abci/abci.md#evidencetype

Describe the solution you'd like
There are additional rpc endpoints that allow causing LightClientAttackEvidence to be entered on-chain and given to the app.
Since LightClientAttackEvidence can encompass different types of Attacks (equivocation, amnesia) these should either be separate endpoints, or an argument for the same endpoint.

I'm trying to use CometMock with penumbra, as a stand-in replacement for the tendermint binary in our integration test suite. Currently, our ABCI service only supports socket communication (although we're investigating adding gRPC support), but I haven't been able to get it working yet. I'm encountering an invalid varint error, which is coming from one of our dependencies relating to protobuf definitions (common when crossing the rust/go boundary over abci).

Here's a branch where I've pushed the current state of play, so it's reproducible by others: penumbra-zone/penumbra#2845 If you're willing to give the Penumbra build a shot, @p-offtermatt, see the steps at https://guide.penumbra.zone/main/pcli/install.html documenting dependencies.

Is your feature request related to a problem? Please describe.
When running a test, it might be that for certain parts you want automatic block production and don't so much care about the exact contents of blocks, and then there is a critical part where you need to control behaviour very precisely.
For example, while IBC connections are set up, you want blocks produced automatically, and then you want full control.

Describe the solution you'd like
It could be helpful to have ways to switch "time control modes" during the execution.
The way that is likely best would be via an rpc endpoint

When I try to build the binary on the v0.34.x branch, I encounter an error:

❯ go install ./cometmock/
# github.com/informalsystems/CometMock/cometmock/abci_client
cometmock/abci_client/client.go:288:106: block.Evidence.Evidence.ToABCI undefined (type "github.com/cometbft/cometbft/types".EvidenceList has no field or method ToABCI)
cometmock/abci_client/client.go:311:118: undefined: abcitypes.Misbehavior
cometmock/abci_client/client.go:711:11: assignment mismatch: 1 variable but a.CurState.MakeBlock returns 2 values

It appears this was introduced by 73bdb02 ; if I revert just that commit, then I'm able to build again.

Builds on main and v0.37.x work just fine, no problems there.

CometMock should feature a Makefile that provides common functionality, like installing, to make it more opaque and streamlined to users.

Related, CometMock should also provide a version command that can be run to check the current version. Naming should likely come from the CometBFT version that it mocks, e.g. v0.37.x