Prove that new publishing slots work in a full-pubset test #385
Conversation
drozdziak1
changed the title
| @@ -193,6 +193,11 @@ pub fn upd_price( | |||
| } | |||
| } | |||
|
|
|||
| solana_program::msg!( | |||
There was a problem hiding this comment.
ugh, I missed this, sorry!
| @@ -536,15 +536,17 @@ impl PythSimulator { | |||
| /// TODO : this fixture doesn't set the product metadata | |||
| pub async fn setup_product_fixture( | |||
There was a problem hiding this comment.
nit: comment should say publishers
There was a problem hiding this comment.
I believe this comment is correct. there's stuff like symbol name in the test case assets, but we only make it available for lookup to distinguish symbols human-readably, without filling in the relevant field in the metadata k/v/ store.
| #[tokio::test] | ||
| async fn test_full_publisher_set() -> Result<(), Box<dyn std::error::Error>> { | ||
| let mut sim = PythSimulator::new().await; | ||
| let pub_keypairs: Vec<_> = (0..PC_NUM_COMP).map(|_idx| Keypair::new()).collect(); |
There was a problem hiding this comment.
you sure this constant has the right value on each network? (i think you may have added a separate test toggled by the feature flag that it does have the expected value, but i forget)
There was a problem hiding this comment.
There's a couple potential problems to discuss:
- The size is not computed right - we already explicitly verify in
test_sizes.rsthat Pythnet has 64 and Solana has 32 publishers. - In the horrifying case that there's still a mismatch between what the C and Rust code is actually using - it should become evident as part of this test:
** If C code is using 64 and Rust is giving it 32 pubs - we would likely see an invalid memory access crash the program. In the worst case where no mem violation happens, the aggregate would partly come from garbage data and fail to yield the desired price value.
** If C code is using 32 and Rust is giving it 64 pubs - there should be an assertion failure due to aggregation working only on the first half of the publisher set, and computing a trivial aggregate price of100with confidence30, instead of the110with conf20that's supposed to come out of the two values "meeting in the middle'
| let (first_third, other_two_thirds) = pub_keypairs.split_at(n_pubs / 3 + 1); | ||
| let (mid_third, last_third) = other_two_thirds.split_at(n_pubs / 3 + 1); | ||
|
|
||
| for (first_kp, mid_kp) in first_third.iter().zip(mid_third.iter()) { |
There was a problem hiding this comment.
I'd feel better if there was test where all 64 publishers publish at the same time
There was a problem hiding this comment.
Here it's only 2/3 at a time
There was a problem hiding this comment.
You are right, I am no longer sure why I went for the 2/3 scenario. I made it just all publishers, with two price values divided 50/50 between the halves. The premise being - if not all publishers are in use, the aggregation should prefer the majority and result in an aggregate closer to it, which should break the assertion.
| sim.upd_price(mid_kp, price, mid_quote).await?; | ||
|
|
||
| slot += 1; | ||
| sim.warp_to_slot(slot).await?; |
There was a problem hiding this comment.
I don't think you need to warp slot here, the slots already move forward while you're doing the slow task of sending all the upd_price transactions
There was a problem hiding this comment.
I dug a bit deeper. Zero slot warps fails to aggregate, but a single slot warp from 1 to 2 works, followed by a single extra update (my dev debug message shows that aggregate got bumped for that last update). I still managed to simplify the slot stuff with that. Thanks!
| // publisher slots on Pythnet are working. Here's how this works: | ||
| // | ||
| // * Fill all publisher slots on a price | ||
| // * Divide the price component array into three parts roughly the |
There was a problem hiding this comment.
This comment became outdated now that you're halving
| // different values, this time using mid_third and last_third without first_third | ||
| // * Verify again for the new values meeting in the middle as aggregate. | ||
| #[tokio::test] | ||
| async fn test_full_publisher_set_two_thirds() -> Result<(), Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
The name of this test is outdated too
Motivation
As yet another method for sanity-checking the increased publisher count on Pythnet, it is useful to check that the new slots are used for aggregation properly, just as well as the pre-existing ones.
Summary of changes
test_full_publisher_set.rs- Add a new test that triggers aggregation on two subsets of a symbol's publishers, proving that for Pythnet, the new publishing slots are used for aggregation. This test is run both for Pythnet and Solana on purpose, to validate that assumpions about aggregate behavior are consistent on both of the networks.utils.rs- Renameget_status_for_updatetoget_status_for_conf_price_ratioto better indicate that it may result in rejecting the updated price in next aggregation.upd_price.rs- Point out that a failedget_status_for_price_conf_ratiomay render the price update rejected from next aggregatepyth_simulator.rs- Modify some setup methods to take a slice of publisher keypairs, making multi-pub setup possible.