chore: ntx-builder subs to block stream + sync before execution#2085
chore: ntx-builder subs to block stream + sync before execution#2085SantiagoPittella wants to merge 9 commits into
Conversation
5562325 to
c765e9a
Compare
test: add test for ntx builder block sync
add ntx install to makefile fix lint remove inflight tx support
8473127 to
e337b7d
Compare
| - [BREAKING] Removed `miden-node ntx-builder` subcommand and created a separate `miden-ntx-builder` binary ([#2067](https://github.com/0xMiden/node/pull/2067)). | ||
| - [BREAKING] Reworked note proto types for multi-attachment support: `NoteMetadata` now carries `attachment_schemes` (repeated) and `attachments_commitment` instead of a single `attachment`. `Note` and `NetworkNote` gained an `attachments` field. `NoteSyncRecord` now embeds full `NoteMetadata` instead of `NoteMetadataHeader`. Removed `NoteAttachmentKind` enum and `NoteMetadataHeader` message ([#2078](https://github.com/0xMiden/node/pull/2078)). | ||
| - [BREAKING] Changed `SyncChainMmr` endpoint: the upper end of the block range we're syncing is now the chain tip with the requested finality level. Validator signature is also returned ([#2075](https://github.com/0xMiden/node/pull/2075)). | ||
| - Switched the network transaction builder from mempool subscription to the store's committed-block subscription: on startup it resumes from its persisted block, catches up to the chain tip before spawning any account actors, and applies committed network-account updates, notes, and nullifiers from each block. Submitted transactions now carry a configurable expiration delta (default 5 blocks). |
There was a problem hiding this comment.
Do we need a changelog entry for this? Need link to PR if we keep it.
There was a problem hiding this comment.
I think we can consider this internal-only refactoring, so not needed.
However the expiration config item should be added I think.
|
Have had a pass of this and LGTM more or less. Will have another pass tomorrow. Quite large change set |
There was a problem hiding this comment.
We shouldn't need any additional endpoints except for this one - so in theory we could yank this service and use the RPC's streaming method only?
But maybe I've missed something. We can also do this in a follow-up PR so its less noisy perhaps..
There was a problem hiding this comment.
I didn't fully removed some of the calls to other endpoints yet. Basically changed the mempool subscription and the sync to use the block subscription but we still use get_current_blockchain_data to get the chain tip but this can be removed if the blockchain subscription also returns the chain tip I think.
Then we have a couple of methods used by our DataStore implementation for execution:
- get_account
- get_vault_asset_witnesses
- get_storage_map_witness
- get_note_script_by_root
And then we use some endpoints for account hydratation:
- get_network_account_details_by_id and get_network_account to fetch the full Account state for an account not yet in the local DB.
- get_unconsumed_network_notes.
We use this last two when we initialize the ntx-builder from an existing node, otherwise we will need to start every new (one with an empty database) ntx-builder from genesis
There was a problem hiding this comment.
If we go only with block subscription from the genesis, during catch up we are only going to accumulate the partial accounts from the beginning to the chain tip instead of just fetching the account state. Should we store the notes from the very beginning despite we have a large amount of blocks to sync? Or should we discard them?
There was a problem hiding this comment.
This PR is a commit on top of this changes moving to only the block subscription #2109
There was a problem hiding this comment.
Let me know wdyt if I should keep that path
There was a problem hiding this comment.
It is inefficient but imo much simpler if we just process block-by-block.
Ideally we would also always be in-sync so what we're "optimising" for here is really a cold start/reset of the ntx builder. Which is okay, but I think not something we should focus on.
I'll checkout the linked PR 👍
| pub fn compile_expiration_tx_script(tx_expiration_delta: u16) -> TransactionScript { | ||
| let script_src = format!( | ||
| "begin\n push.{tx_expiration_delta}\n exec.::miden::protocol::tx::update_expiration_block_delta\nend", | ||
| ); | ||
| CodeBuilder::new() | ||
| .compile_tx_script(script_src.as_str()) | ||
| .expect("expiration tx script should compile") |
There was a problem hiding this comment.
I did not realize this had to be inserted as masm code 🤔 I somehow thought its something you specify on the transaction/builder itself.. But I guess it makes sense,
| /// Network accounts are only ever updated by ntx-builder transactions, so a commitment change | ||
| /// here is equivalent to "some submission for this account has been included in a block." | ||
| WaitForNextBlock { | ||
| submitted_commitment: Word, | ||
| submitted_at: BlockNumber, | ||
| }, |
There was a problem hiding this comment.
Do we have a problem with pass-through transactions maybe? As in, if the network account for whatever reason just acts as a pass-through then the commitment wouldn't change even though the transaction has been committed?
Maybe a better model is
| /// Network accounts are only ever updated by ntx-builder transactions, so a commitment change | |
| /// here is equivalent to "some submission for this account has been included in a block." | |
| WaitForNextBlock { | |
| submitted_commitment: Word, | |
| submitted_at: BlockNumber, | |
| }, | |
| TransactionSubmitted { | |
| transaction: TransactionId, | |
| expires_at: BlockNumber, | |
| }, |
though this might mean we need to track the latest committed transaction ID for a given account?
Or we need to allow inspection of the block by actors and our notify model is overly simplistic?
There was a problem hiding this comment.
Something else.. We should probably store the transaction itself so we can apply its delta to the local account without having to hit the database again.
There was a problem hiding this comment.
We could use backon here already since you're actively changing it :)
| /// Notifies every active actor that a new block has been applied. | ||
| /// | ||
| /// Called once per committed block so that actors waiting on their own submitted transaction | ||
| /// (in [`ActorMode::WaitForNextBlock`](crate::actor::ActorMode)) reliably wake up even when | ||
| /// the block did not touch their account. | ||
| pub fn notify_all(&self) { | ||
| for handle in self.actor_registry.values() { | ||
| handle.notify(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ah.. I didn't think of this 🤔
This is okay for now, but we should figure out a better approach in the future.
| while self.last_applied_block < target { | ||
| let block = self | ||
| .block_stream |
There was a problem hiding this comment.
We should consider adding the chain tip to the block stream so that we can always know from within the stream where we want to get to.
So we would sync until we hit the actual current tip.
Alternatively, what we can do now, is loop in the caller by checking after the catch up ends that the current store chain tip is within a couple of blocks of the latest block streamed.
For example if you need to stream a million blocks, then by the time you get there the chain tip will be like.. a thousand blocks ahead.
There was a problem hiding this comment.
i.e. something like:
let mut local_tip = db.chain_tip.block_number;
loop {
let chain_tip = client.store.latest_block().block_num();
if local_tip + 5 > chain_tip {
break;
}
local_tip = self.catch_up(chain_tip).await;
}
closes #1952
This PR replaces that with a subscription to the store's stream of committed blocks: the builder now derives all network-relevant state: new network notes, network-account updates, and nullifiers directly from
SignedBlocksvia a newCommittedBlockEffects, and only ever sees committed state. Inflight tracking, the mempool replay path, and the purge_inflight startup step are gone, and references to inflight transactions in the DB layer have been replaced with commitment-based bookkeeping.To support this, the NtxBuilder gRPC service gains a
BlockSubscriptionRPC (and removesGetBlockHeaderByNumberremoved), sharing its implementation withStoreReplica::BlockSubscriptionso the ntx-builder can drive everything through a single store client. On startup the builder reads its last applied block from the DB, opens the subscription from that point, and runs a dedicated catch-up phase that drains the stream up to the current chain tip before any account actors are spawned, ensuring actors only ever observe a DB that is consistent with the tip.Submitted network transactions now carry an expiration delta (default 5 blocks, configurable via tx_expiration_delta).