Gmg/refactor gyser rpc #174
Conversation
godmodegalactus
force-pushed
the
gmg/refactor_gyser_rpc
branch
from
8917921 to
12ff865
Compare
| @@ -0,0 +1,9 @@ | |||
| use solana_lite_rpc_core::streams::{ | |||
| fn process_block( | ||
| block: SubscribeUpdateBlock, | ||
| commitment_config: CommitmentConfig, | ||
| ) -> ProcessedBlock { |
There was a problem hiding this comment.
is ProcessedBlock correct term in the sense that it's refering to the commitment status processed. the method is producing blocks for other status too - right?
| } | ||
| }; | ||
| } | ||
| bail!("gyser slot stream ended"); |
| let signature = signatures[0]; | ||
| let compute_units_consumed = meta.compute_units_consumed; | ||
|
|
||
| let message = VersionedMessage::V0(v0::Message { |
There was a problem hiding this comment.
could you explain why we do not need to handle "Legacy"
There was a problem hiding this comment.
Here we create a new message from the data we get from gyser, This is also compatible with legacy transactions.
| .collect(), | ||
| }); | ||
|
|
||
| let legacy_compute_budget = message.instructions().iter().find_map(|i| { |
There was a problem hiding this comment.
.find (first) is IMO an anti-pattern: in case there is more than one match it picks the first (random) one. rather it should fail to point out the ambiguity
suggest to use this instead:
.at_most_one()?
There was a problem hiding this comment.
I think this case is handled by the cluster itself. If there is more than one Request Units instruction cluster will not process the transaction. If the transaction is in block we need to p[process it in any case. Having more than one request units instruction is not a real blocker.
| } | ||
| let schedule = self.leader_schedule.read().await; | ||
|
|
||
| let schedule = if schedule.is_empty() |
There was a problem hiding this comment.
that logic I would have not expected here. could you comment why this repair/rebuild is necessary. it should go into a ".repair_if_necessary" method IMO
| } else { | ||
| schedule | ||
| }; | ||
| let ls = schedule |
There was a problem hiding this comment.
ls -> should be renamed
| }; | ||
| let ls = schedule | ||
| .iter() | ||
| .filter(|x| x.leader_slot >= from && x.leader_slot <= to) |
There was a problem hiding this comment.
dumb question, but is it guaranteed that ls cover the complete range of slots?
| }; | ||
|
|
||
| log::info!( | ||
| "Idenity stakes {}, {}, {}, {}", |
|
|
||
| for tx in block.txs { | ||
| // | ||
| data_cache.txs.update_status( |
There was a problem hiding this comment.
don't want to annoy but block sources might disagree about the status of its transactions: one validator might report confirmed where another one reports processed. iff that's the case the .update_status method should either
- refuse to overwrite
- apply a merge with higher-status-wins logic
Refactoring out lite-rpc so that rpc services are independent and everything is streamed into rpc.