feat(indexer): Introduce basic optimistic indexing #7235
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 4 Skipped Deployments
|
iota-ci
added
infrastructure
tomxey
force-pushed
the
sc-platform/introduce-basic-optimistic-indexing
branch
from
e4c92fa to
77a6f95
Compare
tomxey
force-pushed
the
sc-platform/introduce-basic-optimistic-indexing
branch
from
77a6f95 to
61689b2
Compare
| let iota_transaction_response = match request_type { | ||
| None | Some(ExecuteTransactionRequestType::WaitForEffectsCert) => { | ||
| let mut node_response = self | ||
| .fullnode | ||
| .execute_transaction_block(tx_bytes, signatures, options.clone(), request_type) | ||
| .await | ||
| .map_err(error_object_from_rpc)?; | ||
| // it's not locally executed in indexer, no matter what is the status in the | ||
| // node | ||
| node_response.confirmed_local_execution = Some(false); | ||
| node_response | ||
| } | ||
| Some(ExecuteTransactionRequestType::WaitForLocalExecution) => { | ||
| self.execute_and_optimistically_index_tx_effects( | ||
| tx_bytes, | ||
| signatures, | ||
| options.clone(), | ||
| ) | ||
| .await? | ||
| } | ||
| }; |
There was a problem hiding this comment.
We don't need to bind the logic to the request_type that is defined in the context of the node.
Instead it is ok to execute always optimistically here.
There was a problem hiding this comment.
Users could gain a bit of performance that way by not requesting local execution if they don't need it.
Or do we assume that anyone who wants performance will just use the node directly?
| // TODO: shouldn't return type below be from rust-sdk types? | ||
| // Is this type correct? |
There was a problem hiding this comment.
The iota-rest-api is producing return value using type defined in
which contains TransactionEffects that comes from iota-rust-sdk.The return value is serialized to bcs here:
Then the value is deserialized from bcs in
iota/crates/iota-rest-api/src/client/mod.rs
Line 123 in 43412a9
But the type used here is
iota/crates/iota-rest-api/src/client/mod.rs
Line 139 in 43412a9
That's how the client was implemented before, and it works so far, but I wonder what guarantees we have about this.
| sql_query( | ||
| r#" | ||
| INSERT INTO tx_global_order (tx_digest, global_sequence_number) | ||
| SELECT $1, MAX(tx_sequence_number) FROM tx_digests | ||
| ON CONFLICT (tx_digest) DO NOTHING | ||
| "#, | ||
| ) | ||
| .bind::<sql_types::Bytea, _>(&tx_digest_bytes) |
There was a problem hiding this comment.
You can insert a TxGlobalOrder value directly, no? Why using the raw query?
There was a problem hiding this comment.
Raw query makes it possible to do the read of tx_digests and insert in one query, otherwise it would be split into two queries.
| sql_query( | ||
| r#" | ||
| INSERT INTO tx_global_order (tx_digest, global_sequence_number) | ||
| SELECT $1, MAX(tx_sequence_number) FROM tx_digests | ||
| ON CONFLICT (tx_digest) DO NOTHING | ||
| "#, | ||
| ) | ||
| .bind::<sql_types::Bytea, _>(&tx_digest_bytes) | ||
| .execute(conn) | ||
| }, | ||
| Duration::from_secs(30) | ||
| )?; | ||
|
|
||
| run_query_async!(&pool, |conn| { | ||
| tx_global_order::table | ||
| .select(TxGlobalOrder::as_select()) | ||
| .filter(tx_global_order::tx_digest.eq(tx_digest_bytes)) | ||
| .first::<TxGlobalOrder>(conn) | ||
| }) |
There was a problem hiding this comment.
You can use returning to get the assigned value in one go.
There was a problem hiding this comment.
It would be possible, but with a bit more complex raw query, since with a basic one no value would be returned in case row is already present (ON CONFLICT (tx_digest) DO NOTHING).
I'll keep this comment on hold until it is decided what to do with remaining comments in this function.
| IndexedTransaction, | ||
| TxIndex, | ||
| Vec<IndexedEvent>, | ||
| Vec<EventIndex>, | ||
| BTreeMap<String, StoredDisplay>, | ||
| TransactionObjectChangesToCommit, |
There was a problem hiding this comment.
Consider wrapping into a type TransactionDataToCommit for conciseness.
There was a problem hiding this comment.
You can then also wrap CheckpointTransaction into a TransactionExtractor, implement methods to extract each individual component to index and implement TryFrom or From for CheckpointTransaction.
There was a problem hiding this comment.
I think creting TransactionDataToCommitis a good idea.
Not sure about the TransactionExtractor though. Not all components that are being indexed are extracted independently, so it seems to me that implementation of the extractor would not be that simple.
| Ok(()) | ||
| } | ||
|
|
||
| fn optimistic_event_indices_from_indexed_event_indices( |
There was a problem hiding this comment.
Please consider simplifying: Copilot suggests
fn optimistic_event_indices_from_indexed_event_indices(
event_indices: Vec<EventIndex>,
) -> OptimisticEventIndices {
let splits: Vec<_> = event_indices.into_iter().map(|i| i.split()).collect();
OptimisticEventIndices {
optimistic_event_emit_packages: splits.iter().map(|t| t.0.clone().into()).collect(),
optimistic_event_emit_modules: splits.iter().map(|t| t.1.clone().into()).collect(),
optimistic_event_senders: splits.iter().map(|t| t.2.clone().into()).collect(),
optimistic_event_struct_packages: splits.iter().map(|t| t.3.clone().into()).collect(),
optimistic_event_struct_modules: splits.iter().map(|t| t.4.clone().into()).collect(),
optimistic_event_struct_names: splits.iter().map(|t| t.5.clone().into()).collect(),
optimistic_event_struct_instantiations: splits.iter().map(|t| t.6.clone().into()).collect(),
}
}There was a problem hiding this comment.
There is some cloning in the simplified version, but I suppose it should be ok. Applying.
| store: PgIndexerStore, | ||
| prometheus_registry: &Registry, | ||
| reader: IndexerReader, | ||
| config: &IndexerConfig, | ||
| custom_runtime: Option<Handle>, | ||
| metrics: IndexerMetrics, |
There was a problem hiding this comment.
This breaks the public API of the library.
Alternatively we can add a new OptimisticWriteApi that wraps WriteApi and holds the new optimistic methods.
Then we can add a new function here with the new signature to build the optimistic rpc-server.
There was a problem hiding this comment.
Is this library intended to be used by users outside of IF?
Description of change
Introduce basic optimistic indexing logic.
This basic implementation doesn't solve all known issues (race conditions, dependency checks, pruning, etc.), those will be solved later on the feature branch.
Links to any relevant issues
fixes #7212
How the change has been tested
Describe the tests that you ran to verify your changes.
Make sure to provide instructions for the maintainer as well as any relevant configurations.
Infrastructure QA (only required for crates that are maintained by @iotaledger/infrastructure)
Release Notes