-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(sequencer): integrate slinky oracle and vote extension logic #1236
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers remaining as far as I'm concerned, but still a few unresolved comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many more comments.
market_map::v1::{ | ||
GenesisState as MarketMapGenesisState, | ||
GenesisStateError as MarketMapGenesisStateError, | ||
}, | ||
oracle::v1::{ | ||
GenesisState as OracleGenesisState, | ||
GenesisStateError as OracleGenesisStateError, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of aliasing the types, can you alias the module instead? Like so:
market_map::v1::{ | |
GenesisState as MarketMapGenesisState, | |
GenesisStateError as MarketMapGenesisStateError, | |
}, | |
oracle::v1::{ | |
GenesisState as OracleGenesisState, | |
GenesisStateError as OracleGenesisStateError, | |
}, | |
market_map::v1 as market_map, | |
oracle::v1 as oracle, |
Then you can refer to the types as market_map::GenesisState
and oracle::GenesisState
. I prefer to restrict type aliases to cases like hiding type parameters that would be repeated everywhere (as in the case of anyhow::Result<T> = Result<T, anyhow::Error>
.
@@ -712,6 +910,25 @@ mod tests { | |||
bridge_sudo_change_fee: Some(24.into()), | |||
ics20_withdrawal_base_fee: Some(24.into()), | |||
}), | |||
slinky: Some( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you fill out the genesis data here? This is used in the snapshot test below and it would be great to catch if this changed.
There is a unit test further down (can't comment on it because it's outside the diff range), mismatched_addresses_are_caught
, which could also benefit from a check on the market maker address.
SlinkyGenesis { | ||
market_map: MarketMapGenesisState { | ||
market_map: MarketMap { | ||
markets: IndexMap::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For filling in some data here, there is a nice utility crate that provides hashmap!
and btreemap!
similar to vec!
in the standard library: https://docs.rs/maplit/latest/maplit/
And indexmap itself already provides this itself: https://docs.rs/indexmap/latest/indexmap/macro.indexmap.html
"admin": "astria1rsxyjrcm255ds9euthjx6yc3vrjt9sxrm9cfgm" | ||
} | ||
}, | ||
"oracle": {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented on the file where the test that led to this snap was defined: would be great to populate this with some extra data.
type Err = CurrencyPairParseError; | ||
|
||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
let re = regex::Regex::new(r"^([a-zA-Z]+)/([a-zA-Z]+)$").expect("valid regex"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now this regex would be recompiled on every parse, which is not very efficient!
Have a look at what we are doing inside composer to avoid that at the bottom of this comment. However, don't use once_cell's Lazy or the new std LazyLock. Both of them block their respective thread which would be bad in an async context.
Instead, use https://doc.rust-lang.org/stable/std/sync/struct.OnceLock.html, which does not:
static CURRENCY_PAIR_REGEX: OnceLock<Regex> = OnceLock::new();
fn currency_pair_regex() -> &Regex {
CURRENCT_PAIR_REGEX.get_or_init(|| regex::Regex::new(r"^([a-zA-Z]+)/([a-zA-Z]+)$").expect("valid regex"))
}
Also take a look at how we defined the regex in composer, which gives names to the capture groups so its self-documenting. :)
astria/crates/astria-composer/src/rollup.rs
Lines 36 to 60 in 0ed3c19
static ROLLUP_RE: Lazy<Regex> = Lazy::new(|| { | |
Regex::new( | |
r"(?x) | |
^(?P<rollup_name>[[:alnum:]-]+?) | |
# lazily match all alphanumeric ascii and dash; | |
# case insignificant, but we will lowercase later | |
:: | |
(?P<url>.+) | |
# treat all following chars as the url without any verification; | |
# if there are bad chars, the downstream URL parser should | |
# handle that | |
$ | |
", | |
) | |
.unwrap() | |
}); | |
let caps = ROLLUP_RE.captures(from).ok_or_else(ParseError::new)?; | |
// Note that this will panic on invalid indices. However, these | |
// accesses will always be correct because the regex will only | |
// match when these capture groups match. | |
let rollup_name = caps["rollup_name"].to_string().to_lowercase(); | |
let url = caps["url"].to_string(); | |
Ok(Self { | |
rollup_name, | |
url, |
} | ||
|
||
#[instrument(name = "MarketMapComponent::end_block", skip(_state))] | ||
async fn end_block<S: StateWriteExt + 'static>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: StateWrite
} | ||
|
||
#[instrument(skip_all)] | ||
async fn get_currency_pair_mapping(&self) -> Result<HashMap<u64, CurrencyPair>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut currency_pairs = HashMap::new(); | ||
|
||
let mut stream = std::pin::pin!(self.prefix_keys(&prefix)); | ||
while let Some(Ok(key)) = stream.next().await { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is potentially a source of latency: use futures::TryStreamExt::try_collect
on a futures::StreamExt::buffered
adaptor to do this concurrently.
} | ||
|
||
#[instrument(skip_all)] | ||
async fn get_all_currency_pairs(&self) -> Result<Vec<CurrencyPair>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, implement this as a stream and let the call site decide what to do with this. See #1453 for how to do that.
@@ -15,6 +17,7 @@ message GenesisAppState { | |||
IbcParameters ibc_parameters = 8; | |||
repeated string allowed_fee_assets = 9; | |||
Fees fees = 10; | |||
SlinkyGenesis slinky = 11; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I have changed my mind. We should probably call this slinky because there is nothing generic about this. (Or should we use their new *-connect
name already?)
|
||
pub(crate) async fn get_max_num_currency_pairs<S: StateReadExt>( | ||
state: &S, | ||
is_proposal_phase: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This low level function should not be aware of what happens at the higher level, I think. I looked at the is_proposal_phase
function and had to follow it all the way here. IMO move this logic to the call-site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is how it's done in the original: https://github.com/skip-mev/connect/blob/793b2e874d6e720bd288e82e782502e41cf06f8c/abci/strategies/currencypair/default.go#L120 would it be clearer if i just moved this function to vote_extension.rs
? since this is only used for vote extension verification
}) | ||
} | ||
|
||
pub(crate) async fn verify_vote_extension<S: StateReadExt>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intended that if the oracle is disabled, that this function is still called? If the oracle is disabled, NO_ORACLE=true
, I would expect that a sequencer node completely abstains from participating in vote extensions.
I think it might be clearer if we separated the config in
- participates in vote extensions,
- only verifies vote extensions,
- ignores vote extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the oracle sidecar is disabled, this function should still be called as it's part of consensus, and validator nodes don't need the oracle sidecar to perform verification.
the cases are validator nodes which run the oracle sidecar, those that don't, and full nodes. all validator nodes need to perform verification for consensus reasons, however it's optional as to whether they want to run the sidecar which publishes prices to the network as vote extensions. validators shouldn't ignore vote extension verification, as that could result in them accepting an invalid block. full nodes don't care about vote extensions, to them the VEs are just another transaction in a block (the one the proposer now places first).
…1470) Extensions on `cnidarium::StateRead` should implement a stream rather than allocate and collect into datastructures.
This patch enforces more type strictes on slinky oracle domain types: 1. Nonces, Ids, and Prices are new-type wrappers around their respective primtives; This is to avoid using `u128` in place of a `Price`, for example. 2. All oracle types written to state get their own types that themselves derive the borsh serialization traits. 3. All json is removed from the oracle state read and write extension trait. 4. A lot of validation code is moved from sequencer to core so that validation happens closer to the boundary. 5. Functions in the public interface for constructing various validation errors were removed. 6. Constructors that would allow violating invariants (specifically invariants around the contents of currency pairs) were removed. A bug was fixed in the `oracle::StateReadExt::put_price_for_currency_pair` trait method, which was not updating the nonce.
Summary
integrate skip's slinky oracle service into astria.
at a high level, slinky consists of an oracle sidecar program, which interacts with a validator node to provide price data, and various cosmos-sdk modules.
since astria isn't cosmos, the relevant cosmos modules (x/marketmap and x/oracle) were essentially ported into the
slinky
module of the sequencer app, which consists of two components,market_map
andoracle
.the sequencer app was updated to talk to the sidecar during the
extend_vote
phase of consensus to gather prices to put into a vote extension.the vote extension validation logic, proposal logic, and finalization logic were also ported from slinky.
Background
we want oracle data to be available to rollups (and maybe on the sequencer itself too?)
Changes
market_map
andoracle
valuesmarket_map
component for the sequenceroracle
component for the sequencer and the query service for this componentextend_vote
logic which gets the prices from the sidecar and turns them into a vote extensionverify_vote_extension
logic which performs basic validation on a vote extension during the consensus phaseprepare_proposal
logic which gathers the vote extensions from the previous block, prunes any invalid votes, and performs additional validation to create a valid set of VEsprocess_proposal
logic which validates the set of VEs proposed, checking signatures and that the voting power is >2/3 amongst other thingsfinalize_block
logic which writes the updated prices to state based on the commited vote extensions. skip uses stake-weighted median to calculate the final price, but we don't have stake-weighting yet, so i just took the median.Msg
types as sequencer actions (follow-up)SequencerBlockHeader
to contain the extended commit info + a proof for it (also follow-up)DeltaCurrencyPairStrategy
- right now only theDefaultCurrencyPairStrategy
is implemented. can also do in follow-upTesting
TODO: run this on a multi-validator network also
clone slinky: https://github.com/skip-mev/slinky/tree/main
install go 1.22
build and run slinky:
checkout
noot/slinky
branch of astriarun sequencer app w/ grpc port 9090 and
ASTRIA_SEQUENCER_ORACLE_ENABLED=true
rm -rf /tmp/astria_db rm -rf ~/.cometbft rm app-genesis-state.json just run just run-cometbft
should see a sequencer log like:
should see a slinky log like:
then, when blocks are made, should see logs like the following for each block:
Breaking Changelist