basic chain using tower-abci and penumbra stack #5

noot · 2023-05-17T03:43:39Z

Implements a basic ABCI application that communicates with a tendermint process. The application only has account logic at this stage, and only one transaction type accounts::transaction::Transaction which represents a value transfer. It uses penumbra_storage::Storage as the state storage, and penumbra_component::Component to define an application component (in this case, there is only the AccountsComponent).

It uses the tower_abci::Service as the base layer which talks to tendermint. The application logic is built up around the requests made by tendermint.

Currently, there are 3 accounts that have a hard-coded balance of 10e18 at genesis: alice, bob, and carol.

Usage:

cd crates/astria-sequencer && cargo build && ../../target/debug/astria-sequencer --genesis-file=test-genesis.json

Clone and build https://github.com/astriaorg/cometbft (make install && make install_abci)

cometbft init
cometbft node

You can use abci-cli to query the application, for example, to query Alice's nonce: (note that the 0xdeadbeef is totally arbitrary, abci-cli requires some hex string as an arg :/)

$ abci-cli query --path=accounts/nonce/alice 0xdeadbeef
I[2023-05-18|22:21:48.105] service start                                module=abci-client msg="Starting socketClient service" impl=socketClient
-> code: OK
-> height: 6
-> key: accounts/nonce/alice
-> key.hex: 6163636F756E74732F6E6F6E63652F616C696365
-> value: 
-> value.hex: 01000000

You can also curl endpoints directly:

$ curl http://localhost:26657/abci_info
{"jsonrpc":"2.0","id":-1,"result":{"response":{"data":"astria_sequencer","version":"0.1.0","app_version":"1","last_block_height":"1"}}}

After node has started, submit a tx:

$ curl http://localhost:26657/broadcast_tx_commit?tx=0x0003000000626f6205000000616c6963651516050000000000000000000000000001000000
{"jsonrpc":"2.0","id":-1,"result":{"check_tx":{"code":0,"data":null,"log":"","info":"","gas_wanted":"0","gas_used":"0","events":[],"codespace":"","sender":"","priority":"0","mempoolError":""},"deliver_tx":{"code":0,"data":null,"log":"","info":"","gas_wanted":"0","gas_used":"0","events":[],"codespace":""},"hash":"5CD86628A864412D505C5B575EE939F7B60AE507119A0F63749AFD2384B8BAEE","height":"8"}}

Note: the tx was generated with the test test_transaction; will need to make a prettier CLI for this

Inside cometbft you'll see the tx was included:

I[2023-05-17|22:06:46.600] received proposal                            module=consensus proposal="Proposal{2/0 (7640BDA8191E35BD9028FE6913D9C831ED095782B087E45C90905FDD476785FA:1:214894D427A1, -1) 94CE5F23AEE1 @ 2023-05-18T02:06:46.598356671Z}"
I[2023-05-17|22:06:46.601] received complete proposal block             module=consensus height=2 hash=7640BDA8191E35BD9028FE6913D9C831ED095782B087E45C90905FDD476785FA
I[2023-05-17|22:06:46.606] finalizing commit of block                   module=consensus height=2 hash=7640BDA8191E35BD9028FE6913D9C831ED095782B087E45C90905FDD476785FA root=35EE717EA57E28688B0DE0653C38247AAD65BF943D00EF3ADAF5023D2F1FF0A2 num_txs=1
I[2023-05-17|22:06:46.698] executed block                               module=state height=2 num_valid_txs=1 num_invalid_txs=0
I[2023-05-17|22:06:51.750] committed state                              module=state height=2 num_txs=1 app_hash=A0E6C663281AB4E36F7DDC647D192D502F2EE481A4CDE6D8C45B2B51658AD7DD
I[2023-05-17|22:06:51.753] indexed block exents                         module=txindex height=2

You'll also see a log inside the sequencer app:

2023-05-19T02:21:50.047549Z  INFO astria_sequencer::app: executed transaction tx=AccountsTransaction(Transaction { to: "bob", from: "alice", amount: 333333, nonce: 1 }) height=8

Query Alice's nonce again

$ abci-cli query --path=accounts/nonce/alice 0x00
I[2023-05-18|22:26:50.206] service start                                module=abci-client msg="Starting socketClient service" impl=socketClient
-> code: OK
-> height: 62
-> key: accounts/nonce/alice
-> key.hex: 6163636F756E74732F6E6F6E63652F616C696365
-> value: 
-> value.hex: 0101000000

Nonce should increase after the tx was sent :D (note the value is borsh-serialized, so doesn't correspond to the raw value exactly)

You can set the path to accounts/balance/(alice, bob, carol) to check balances also

this steps just moves everything out of `sources/` and places it into `crates/`. `cargo check` passes after adding making the crates workspace members in the root `Cargo.toml`. The crates do not refer to each other by path, and they are also not yet cleaned up.

noot · 2023-06-09T18:36:57Z

Big things that I would like to get done before we merge this into main:
no panics except for where invariants are violated (that's the (&mut Arc)::get_mut calls)
error context for all errors through anyhow::Context::context (note that that's different from eyre::WrapErr::wrap_err)
replace manual parsing of primitives by newtype wrappers + deriving borsh or serde (and then use borsh directly or bincode for serde)
instrument all functions

@SuperFluffy I think these are all addressed; the only thing that may not be fully addressed is adding context for every error, as some errors already had context added lower-down on the stack so I wasn't sure if I should re-add another context. let me know if you want literally every error to have additional context and I can add!

joroshiba · 2023-06-14T17:04:01Z

crates/astria-sequencer/src/app.rs

+        state_tx.put_block_height(0);
+
+        // call init_chain on all components
+        AccountsComponent::init_chain(&mut state_tx, &genesis_state).await?;


Should we have some sort of Component manager to define modules and the order they will be called in similar to the Cosmos manager?

the cosmos manager seems more needed because it's an SDK, and it needs to figure out what modules exist at runtime, whereas for us it's fine to just write them in and do it at compile time (this is what penumbra does too)

joroshiba · 2023-06-14T17:28:53Z

crates/astria-sequencer/src/accounts/types.rs

+    Ord,
+    Debug,
+)]
+pub struct Nonce(u32);


Should we use a u64 here? I believe ethereum uses uint64 for nonce, as does cosmos for sequence?

yeah, can update, u64 just seemed a bit large lol

Better to plan on being successful than not!

crates/astria-sequencer/src/accounts/transaction.rs

joroshiba · 2023-06-14T17:39:40Z

crates/astria-sequencer/src/accounts/state_ext.rs

+    }
+
+    #[instrument(skip(self))]
+    fn put_account_nonce(&mut self, address: &Address, nonce: Nonce) -> Result<()> {


Do we need a put_account_nonce or should this have all the increment logic and be increment_account_nonce? Is there ever an instance in which account nonce should go up by more than one at a time?

if we have an increment function then it'll have a get and a put, but usually wherever this function is called there's already a get, so it seems a bit redundant to have 2 gets as we want to minimize reads/writes.

Would we need the get if it was an increment? It seems we are only getting to do the increment.

joroshiba · 2023-06-14T17:41:02Z

crates/astria-sequencer/src/accounts/transaction.rs

+        Ok(())
+    }
+
+    async fn execute<S: StateWriteExt>(&self, state: &mut S) -> Result<()> {


Is there anything preventing the call of execute without having first validated the tx? What happens if there are insufficient funds when this is called?

In general I wonder if structurally we should be moving more of the logic of "tx creates these DB level changes" directly into the state implementation.

yeah, this function is only called inside DeliverTx, which always performs validation (stateless and non-stateless) before executing it. what do you mean by moving it directly into the state implementation?

I just want to make sure we are creating hardened code here. Should it be possible to call execute before validation? If no we should enforce that it isn't possible not hope someone doesn't forget that.

We should have a write-up about how state gets transformed when moving through these components.

joroshiba · 2023-06-14T17:44:04Z

crates/astria-sequencer/src/accounts/transaction.rs

+        let from_balance = state.get_account_balance(&self.from).await?;
+        let from_nonce = state.get_account_nonce(&self.from).await?;
+        let to_balance = state.get_account_balance(&self.to).await?;
+        state.put_account_balance(&self.from, from_balance - self.amount)?;


Maybe this is just that lower level, but it feels like maybe the logic for state transitions which has race possibilities should be handled by the state itself? If putting the balance succeeds but nonce fails, we have issues.

right, so these writes are all happening on a StateDelta, which is basically a list of pending writes that happen during a block's execution. the writes are all batched and only actually written to state once Commit is called. so yeah this effectively is atomic afaik

Can we create an issue to add tests for error cases here?

joroshiba · 2023-06-14T17:44:33Z

crates/astria-sequencer/src/accounts/transaction.rs

+        let from_nonce = state.get_account_nonce(&self.from).await?;
+        let to_balance = state.get_account_balance(&self.to).await?;
+        state.put_account_balance(&self.from, from_balance - self.amount)?;
+        state.put_account_nonce(&self.from, from_nonce + Nonce::from(1))?;


Should we be setting to the nonce of the transaction and validating that it is acceptable?

the validation occurs outside this function in check_stateful (which happens right before execute is called) so I don't think we need to double validate

* mention that gcc12 is required * make items crate-private by default * add direnv rc to gitignore * remove dead code and unnecessary test the tests were essentially checking if the serde derive was successful * more expressive naming in genesis state * straight u128, not f64 cast * move genesis to its own module * no error handling in tests * define an apphash newtype wrapper like in penumbra * move services to their own module * WIP * add tracing to the tower abci server This commit vendors tracing-tower in the same way as penumbra has done. It then propagates the tracing spans as needed. * format tracing-tower * use penumbra tower trace * remove to_bytes, from_bytes, rely on borsh * provide context for everything * allow configuring listen_addr, genesis state through cli * clippy suggestion * rm forgotten uncomment

joroshiba

LGTM. Great start for the chain overall.

I don't consider any of my comments blocking on this, but we should create issues and consider resolving separately.

SuperFluffy · 2023-06-15T09:59:57Z

My last commits fixed compilation in CI. The reason is that CI defines a RUSTFLAGS env var which overrides the setting in .config/cargo.toml.

The other commits are about fixing clippy lints. I also increased the number of lints that are used for astria-sequencer to clippy::pedantic. This is now enforced in a separate step. The other crates will be fixed step by step.

crates/astria-sequencer/src/state_ext.rs

crates/astria-sequencer/src/lib.rs

crates/astria-sequencer/src/service/consensus.rs

SuperFluffy · 2023-06-15T14:39:25Z

Big things that I would like to get done before we merge this into main:
no panics except for where invariants are violated (that's the (&mut Arc)::get_mut calls)
error context for all errors through anyhow::Context::context (note that that's different from eyre::WrapErr::wrap_err)
replace manual parsing of primitives by newtype wrappers + deriving borsh or serde (and then use borsh directly or bincode for serde)
instrument all functions

@SuperFluffy I think these are all addressed; the only thing that may not be fully addressed is adding context for every error, as some errors already had context added lower-down on the stack so I wasn't sure if I should re-add another context. let me know if you want literally every error to have additional context and I can add!

To document for posteriority: I added context to literally every error I could find.

SuperFluffy · 2023-06-15T15:07:35Z

Cargo.toml

 tonic = "0.9"
 tracing = "0.1"
 tracing-subscriber = "0.3"
 uuid = "1.3.1"
 which = "4.4.0"
+
+[patch."https://github.com/penumbra-zone/penumbra.git"]
+penumbra-storage = { git = "https://github.com/astriaorg/penumbra.git", branch = "noot/fix-trace-v0.53.1" }


can you open PRs against penumbra and open an issue here so that we can track it?

SuperFluffy

Approving to unblock this.

Opening an issue to fix rocksdb in CI and docker. See also these github issues:

rust-rocksdb/rust-rocksdb#310 (comment)
rust-rocksdb/rust-rocksdb#354 (comment)

SuperFluffy and others added 10 commits May 16, 2023 15:14

use the astria- prefix on all crates

3393a04

remove overlooked hidden files, licenses

edbb591

make all astria dependencies paths

4b3499e

apply fmt to the entire workspace

70ee28a

begin astria-sequencer; implement base app

9a42626

begin app and account component

38b3491

begin connecting consensus component to app

fb75596

cleanup and fix

03e5109

update readme

878b751

SuperFluffy force-pushed the local_path_dependencies branch from 70ee28a to ee39c01 Compare May 17, 2023 10:08

Base automatically changed from local_path_dependencies to main May 17, 2023 10:09

noot added 12 commits May 17, 2023 15:21

merge

f1e6367

add transaction wrapper, StateReadExt/StateWriteExt, finish up app logic

0d15225

wip: testing with transactions

1b7c722

refactor ConsensusService, fix referencing issues

bb1bcbc

don't panic on deliver_tx fail

5206efb

init genesis state on if init_chain not called

1f40ac0

implement nonce; clean up account module format

ca98fa7

clippy

a63134d

implement account queries, wip

0f5c452

fix state apply after tx.execute

0a026a6

change default logging to info

8051662

implement mempool CheckTx

2479af3

noot changed the title ~~[wip] basic chain using tower-abci and penumbra stack~~ basic chain using tower-abci and penumbra stack May 19, 2023

noot marked this pull request as ready for review May 19, 2023 02:38

noot requested review from joroshiba, SuperFluffy, steezeburger and itamarreif May 19, 2023 02:59

noot requested a review from SuperFluffy June 9, 2023 18:35

joroshiba reviewed Jun 14, 2023

View reviewed changes

SuperFluffy and others added 3 commits June 14, 2023 19:38

use patch of penumbra-tower-trace, update README

d729d47

Merge branch 'main' of github.com:astriaorg/astria into noot/sequencer

74a80c4

joroshiba approved these changes Jun 15, 2023

View reviewed changes

SuperFluffy added 9 commits June 15, 2023 10:55

add tokio_unstable to ci rust flags

2db1630

add quotes

52b7f88

all rustflag components in one line

4e63136

restrict visibility of items to crate-private

c6ff33a

restrict visibility of items to crate-private

4655f9f

fix clippy pedantic suggestions

b5ccc38

run pedantic clippy against sequencer

4e78132

too many dashes in cli opt

40a284c

set astria-sequencer clippy step to pedantic

f6a2a76

SuperFluffy added 3 commits June 15, 2023 12:35

fix clippy in tests

fe3d2c0

install clang in dockerfiles

c9613ea

remove let annotation

048964c

SuperFluffy reviewed Jun 15, 2023

View reviewed changes

properly handle failure to process consensus request

7cdd9d2

clippy suggestion

c2b2b5d

SuperFluffy reviewed Jun 15, 2023

View reviewed changes

SuperFluffy approved these changes Jun 15, 2023

View reviewed changes

SuperFluffy mentioned this pull request Jun 15, 2023

Use system rocksdb in CI and docker #80

Closed

noot merged commit 63ef810 into main Jun 15, 2023
8 checks passed

noot deleted the noot/sequencer branch June 15, 2023 17:41

SuperFluffy mentioned this pull request Jun 4, 2024

conductor blackbox test for soft-and-firm mode is flaky #1143

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic chain using tower-abci and penumbra stack #5

basic chain using tower-abci and penumbra stack #5

noot commented May 17, 2023 •

edited

Loading

noot commented Jun 9, 2023

joroshiba Jun 14, 2023

noot Jun 14, 2023

joroshiba Jun 14, 2023

noot Jun 14, 2023

joroshiba Jun 15, 2023

joroshiba Jun 14, 2023

noot Jun 14, 2023

joroshiba Jun 15, 2023

joroshiba Jun 14, 2023

noot Jun 14, 2023

joroshiba Jun 15, 2023

SuperFluffy Jun 15, 2023

joroshiba Jun 14, 2023

noot Jun 14, 2023

joroshiba Jun 15, 2023

joroshiba Jun 14, 2023

noot Jun 14, 2023

joroshiba left a comment

SuperFluffy commented Jun 15, 2023

SuperFluffy commented Jun 15, 2023

SuperFluffy Jun 15, 2023

SuperFluffy left a comment

basic chain using tower-abci and penumbra stack #5

basic chain using tower-abci and penumbra stack #5

Conversation

noot commented May 17, 2023 • edited Loading

noot commented Jun 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joroshiba left a comment

Choose a reason for hiding this comment

SuperFluffy commented Jun 15, 2023

SuperFluffy commented Jun 15, 2023

Choose a reason for hiding this comment

SuperFluffy left a comment

Choose a reason for hiding this comment

noot commented May 17, 2023 •

edited

Loading