fix: split system transaction executor into per-pallet allow-listed traits - #2080
fix: split system transaction executor into per-pallet allow-listed traits#2080ozgb wants to merge 2 commits into
Conversation
…raits Replace the single MidnightSystemTransactionExecutor trait with MidnightSystemTransactionCNightExecutor and MidnightSystemTransactionBridgeExecutor, so pallet-cnight-observation and pallet-c2m-bridge can each only submit the SystemTransaction variants they actually construct, instead of being able to pass through any variant the ledger recognizes. Each new trait is backed by a dedicated ledger host function (apply_cnight_system_transaction / apply_bridge_system_transaction) that deserializes once and checks its allow-list before applying, rather than a separate classifier call ahead of apply_system_transaction. The governance extrinsic path is rebuilt the same way onto a new apply_governance_system_transaction host function. The previous apply_system_transaction and is_governance_allowed_system_tx host functions are kept in place, unused by new code, since they remain part of the node<->runtime ABI for already-published runtime WASM and historical block replay. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
ozgb
added
skip-changes-check-issue
bot:ai-assisted
Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 800158528e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| #[codec(index = 15)] | ||
| SystemTransactionNotAllowedForCNight, | ||
| #[codec(index = 16)] | ||
| SystemTransactionNotAllowedForBridge, |
There was a problem hiding this comment.
Adding these pallet error variants changes the runtime metadata, but both metadata/static/midnight_metadata.scale and metadata/static/midnight_metadata_2.1.0.scale remain unchanged. Since metadata/src/lib.rs:17 generates midnight_metadata_latest from the latter file, consumers of the metadata crate cannot identify or decode the new module error indices 15 and 16 when either allow-list rejects a transaction; regenerate both current metadata artifacts with the repository's rebuild target.
AGENTS.md reference: AGENTS.md:L146-L149
Useful? React with 👍 / 👎.
|
1 participant
Overview
MidnightSystemTransactionExecutor::execute_system_transactionlet a caller applyany serialized
SystemTransactionvariant to the ledger, with no restriction.The root-origin
send_mn_system_transactionextrinsic already gated on anallow-list (only
OverwriteParameterspasses), but the trait path used bypallet-cnight-observationandpallet-c2m-bridgebypassed that gate entirely.Both callers only ever construct a fixed set of transaction kinds today, so this
was latent rather than exploited, but nothing at the trait boundary stopped a
future bug or compromised caller from submitting any other ledger-recognized
variant (e.g. treasury payouts, reserve distribution) through either pallet.
This replaces the single shared trait with per-caller traits, each restricted at
the trait boundary to the system transaction variants that pallet is actually
allowed to construct:
MidnightSystemTransactionCNightExecutor(cnight-observation): onlyCNightGeneratesDustUpdate.MidnightSystemTransactionBridgeExecutor(c2m-bridge): onlyUnlockToTreasury,DistributeReserve, andDistributeNight(ClaimKind::CardanoBridge, _).(
OverwriteParametersonly).Each is backed by a dedicated ledger host function
(
apply_governance_system_transaction,apply_cnight_system_transaction,apply_bridge_system_transaction) that deserializes the transaction once andchecks its allow-list before applying, rather than a separate classifier call
ahead of
apply_system_transaction(which would deserialize and cross theWASM host-function boundary twice per call). The previous
apply_system_transactionandis_governance_allowed_system_txhost functionsare kept in place, unused by new pallet code, since host functions are part of
the node<->runtime ABI and removing one an already-published runtime WASM can
call would break replay of historical blocks.
runtime/src/lib.rsneeds no changes:pallet-midnight-systemnow implementsboth new traits, and the runtime's existing
type MidnightSystemTransactionExecutor = MidnightSystem;wiring for bothpallets keeps compiling as-is.
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
cargo check -p pallet-midnight-system -p pallet-cnight-observation -p pallet-c2m-bridge -p midnight-primitives -p midnight-node-runtime -p midnight-node-ledgercargo test -p pallet-midnight-system -p pallet-cnight-observation -p pallet-c2m-bridge— all pass, including 5 new unit tests inpallet-midnight-systemthat construct real system transactions against a real ledger (viainit_storage_paritydb_separate) and assert each executor's allow-list accepts/rejects the right variants.cargo test -p midnight-node-ledger(error-code uniqueness tests, to confirm the newSystemTransactionError::NotAllowedForCallervariant doesn't collide with an existing or retired wire code).cargo clippy/cargo fmton all touched crates.Full workspace
cargo check --all-targets— confirmsruntime/src/lib.rscompiles unmodified.Additional tests are provided (if possible)
🔱 Fork Strategy
Links