feat!: require chain id in transactions #973
Conversation
github-actions
bot
added
proto
joroshiba
force-pushed
the
joroshiba/transaction-format
branch
from
c0c8673
to
8280a6f
Compare
|
LGTM from composer |
There was a problem hiding this comment.
The chain ID is already available when connecting to sequencer by querying it's ABCI /genesis endpoint, which is what conductor is doing.
I would like our services to be consistent in how they query the chain ID and use a single source of truth (the mentioned endpoint).
However, if you prefer to explicitly use a configuration parameter for this (for example to guard against accidentally connecting to the wrong sequencer network), then please still query /genesis and check that configured_chain_id == remote_chain_id to have composer before it receives or polls any transactions.
| @@ -27,6 +27,9 @@ ASTRIA_COMPOSER_API_LISTEN_ADDR="0.0.0.0:0" | |||
| # Address of the RPC server for the sequencer chain | |||
| ASTRIA_COMPOSER_SEQUENCER_URL="http://127.0.0.1:26657" | |||
|
|
|||
| # Chain ID of the sequencer chain which transactions are submitted to. | |||
| ASTRIA_COMPOSER_SEQUENCER_CHAIN_ID="astria-dev-1" | |||
There was a problem hiding this comment.
IMO remove in favor of querying sequencer's /genesis endpoint to extract the cometbft chain ID from there. This is how conductor is doing it and I would like this to be consistent between services.
| @@ -157,6 +173,21 @@ pub(crate) async fn execute<S: StateWriteExt>( | |||
| .await | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
| pub(crate) struct InvalidChainId(pub(crate) String); | |||
There was a problem hiding this comment.
Admittedly I just matched the InvalidNonce error publicity since these should be consumed similarly, it is referenced in a test I added in app.rs to ensure it fails correctly.
There was a problem hiding this comment.
Approving after syncing offline. We decided to a) require Sequencer chain IDs be set in config, and b) the configured Sequencer chain IDs be checked against the /genesis endpoint. However, this will and can be done in follow PRs (for both composer and conductor).
I am approving this PR. Before merge, please address my comments re the ABCI error codes:
- add entries to the
DisplayandFromimpls - add matching protobuf ABCI error codes (note: apparently these have been removed feat(sequencer): check for sufficient balance in
check_tx#869 for some reason)
github-actions
bot
added
ci
|
Follow up issue: #986 |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| // Transactions must match the chain id of the node. | ||
| let chain_id = state.get_chain_id().await?; | ||
| ensure!( | ||
| self.params.chain_id == chain_id.as_str(), | ||
| InvalidChainId(self.params.chain_id.clone()) | ||
| ); | ||
|
|
There was a problem hiding this comment.
you can remove this - the mempool check is sufficient, no txs with invalid chain ids will make it here. the nonce check below is needed for strict equality before execution
There was a problem hiding this comment.
I think we need to protect against a validator running their node with a no op mempool. IE someone can modify code for their validator grab alternate transactions and propose?
| @@ -2111,6 +2182,52 @@ mod test { | |||
| ); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn app_deliver_tx_invalid_chain_id() { | |||
There was a problem hiding this comment.
move/change this test to test the mempool check (after you delete the check_stateful check this test will fail)
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Summary
Adds the concept of
TransactionParamstoUnsignedTransactionas a body that can grow to include many different parameters which much pass for a transaction to be valid, and uses this structure to add achain_idto ensure that transactions from one Astria network cannot be replayed on another.Background
There are likely to be additional parameters we can add onto transactions (such a expiration or things relating to auction bids), and we needed to add the chain id parameter.
Changes
Testing
CI/CD