apollo_l1_provider: add test for low latest L1 block number

[guy-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[guy-starkware]

• papyrus_base_layer: delete l1_endpoint_monitor crate #10736
• papyrus_base_layer: delete monitored base layer #10731
• apollo_l1_provider: re-write FinalityTooHigh error as LatestBlockNumberTooLow #10729
• apollo_l1_provider: use cyclic base layer wrapper instead of monitored base layer #10703
• apollo_l1_provider: make sure wrong event types do not get scraped #10700
• papyrus_base_layer: gate integration tests #10671
• apollo_l1_provider: add test for each type of L1 event we filter on #10595
• papyrus_base_layer: test util for creating an ethereum base layer using a mock provider #10588
• papyrus_base_layer: extract check and cycle URLs into a helper function with logging #10618
• papyrus_base_layer: add CyclicBaseLayerWrapper #10412
• papyrus_base_layer: make BaseLayer trait functions mutable #10404
• papyrus_base_layer: add the option to cycle the URL directly into the base layer #10357
• papyrus_base_layer: add ordered list of provider URLs to the config #10354
• apollo_l1_provider: remove finality_too_high helper function, add MissingBlock error #10348
• apollo_l1_gas_price: add test for low latest L1 block number #10346
• apollo_l1_provider: add test for low latest L1 block number #10345 👈 (View in Graphite)
• apollo_l1_provider: replace mutex with atomic u64 in a scraper test #10344
• papyrus_base_layer: remove finality from base layer #10303
• papyrus_base_layer: remove latest_l1_block from base layer #10300
• apollo_central_sync: remove base layer call to latest_proved_block #10246
• papyrus_base_layer: add test for a base layer without a starknet contract #10242
• main-v0.14.1

This stack of pull requests is managed by Graphite. Learn more about stacking.

[ShahakShama]

@ShahakShama reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @guy-starkware)

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 159 at r1 (raw file):

#[tokio::test]
async fn base_layer_returns_block_number_below_finality() {

Add the expect result to the end of the test (_causes_error)

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 163 at r1 (raw file):

    const FINALITY: u64 = 10;
    const INITIAL_L1_BLOCK_NUMBER: u64 = 100;
    const INITIAL_L1_BLOCK_HASH: L1BlockHash = L1BlockHash([123; 32]);

This is in-fact the hash of all l1 blocks in this test. Either fix it so that you'll have different hash for INITIAL_L1_BLOCK_NUMBER and WRONG_L1_BLOCK_NUMBER or rename this to reflect that it's for all blocks

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 188 at r1 (raw file):

    // Test.
    assert_eq!(scraper.send_events_to_l1_provider().await, Ok(()));

use unwrap instead

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 196 at r1 (raw file):

    assert_matches!(
        scraper.send_events_to_l1_provider().await,
        Err(L1ScraperError::FinalityTooHigh { .. })

Why is that the expected outcome? IIUC this means that if I got block x then got block x-95 then you return finality too high to the provider, but shouldn't you return a reorg error?

[guy-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ShahakShama)

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 159 at r1 (raw file):

Previously, ShahakShama wrote…

Add the expect result to the end of the test (_causes_error)

I'm not sure what you want me to add where?

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 163 at r1 (raw file):

Previously, ShahakShama wrote…

This is in-fact the hash of all l1 blocks in this test. Either fix it so that you'll have different hash for INITIAL_L1_BLOCK_NUMBER and WRONG_L1_BLOCK_NUMBER or rename this to reflect that it's for all blocks

Done.

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 196 at r1 (raw file):

Previously, ShahakShama wrote…

Why is that the expected outcome? IIUC this means that if I got block x then got block x-95 then you return finality too high to the provider, but shouldn't you return a reorg error?

If x is 100 you should get FinalityTooHigh. If the new height is lower than the old height but still above 10, you should get a reorg.

Here are the list of checks we do each scrape, in order:

1. Reorg detection. Assume the same block number, try to get that block again. If the block is missing, or if the hash changed, trigger a reorg. This doesn't check finality. So if the endpoint's latest L1 block number goes down a few blocks, it may still have the block in question, so it may not trigger the reorg error.
2. Check if the latest L1 block number from the endpoint is above finality. This guards against what I assume is a but in Infura where sometimes it reports the latest number is zero (I never checked, it could also be reporting 3 or 8, but it doesn't matter). The get block at number function could still work normally, so it doesn't necessarily trigger reorg. In such cases we cannot continue because the latest number - 10 is not valid. This is FinalityTooHigh error.
3. If we don't trigger these two errors, and still the endpoint reports a latest number (after subtracting finality) that is lower than the number we already scraped (or equal) we just issue a warning and wait until the next scrape (this happens when switching endpoints and the new one -- usually Infura -- is less updated than the old one).

[guy-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ShahakShama)

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 188 at r1 (raw file):

Previously, ShahakShama wrote…

use unwrap instead

Done.

[guy-starkware]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ShahakShama)

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 196 at r1 (raw file):

Previously, guy-starkware wrote…

If x is 100 you should get FinalityTooHigh. If the new height is lower than the old height but still above 10, you should get a reorg.

Here are the list of checks we do each scrape, in order:

1. Reorg detection. Assume the same block number, try to get that block again. If the block is missing, or if the hash changed, trigger a reorg. This doesn't check finality. So if the endpoint's latest L1 block number goes down a few blocks, it may still have the block in question, so it may not trigger the reorg error.
2. Check if the latest L1 block number from the endpoint is above finality. This guards against what I assume is a but in Infura where sometimes it reports the latest number is zero (I never checked, it could also be reporting 3 or 8, but it doesn't matter). The get block at number function could still work normally, so it doesn't necessarily trigger reorg. In such cases we cannot continue because the latest number - 10 is not valid. This is FinalityTooHigh error.
3. If we don't trigger these two errors, and still the endpoint reports a latest number (after subtracting finality) that is lower than the number we already scraped (or equal) we just issue a warning and wait until the next scrape (this happens when switching endpoints and the new one -- usually Infura -- is less updated than the old one).

Correction: if new height is lower but still above 10 you may get a reorg (if the block you last scraped no longer exists) but you may also just get a warning if the block exists but the (finality-adjusted) latest number is not higher than the last one you scraped.

[guy-starkware]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ShahakShama)

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 196 at r1 (raw file):

Previously, guy-starkware wrote…

If x is 100 you should get FinalityTooHigh. If the new height is lower than the old height but still above 10, you should get a reorg.

Here are the list of checks we do each scrape, in order:

1. Reorg detection. Assume the same block number, try to get that block again. If the block is missing, or if the hash changed, trigger a reorg. This doesn't check finality. So if the endpoint's latest L1 block number goes down a few blocks, it may still have the block in question, so it may not trigger the reorg error.
2. Check if the latest L1 block number from the endpoint is above finality. This guards against what I assume is a but in Infura where sometimes it reports the latest number is zero (I never checked, it could also be reporting 3 or 8, but it doesn't matter). The get block at number function could still work normally, so it doesn't necessarily trigger reorg. In such cases we cannot continue because the latest number - 10 is not valid. This is FinalityTooHigh error.
3. If we don't trigger these two errors, and still the endpoint reports a latest number (after subtracting finality) that is lower than the number we already scraped (or equal) we just issue a warning and wait until the next scrape (this happens when switching endpoints and the new one -- usually Infura -- is less updated than the old one).

Correction: if new height is lower but still above 10 you may get a reorg (if the block you last scraped no longer exists) but you may also just get a warning if the block exists but the (finality-adjusted) latest number is not higher than the last one you scraped.

[ShahakShama]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @guy-starkware)

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 159 at r1 (raw file):

Previously, guy-starkware wrote…

I'm not sure what you want me to add where?

Sorry, I should've been more clear
Rename the test to base_layer_returns_block_number_below_finality_causes_error

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 196 at r1 (raw file):

Previously, guy-starkware wrote…

Correction: if new height is lower but still above 10 you may get a reorg (if the block you last scraped no longer exists) but you may also just get a warning if the block exists but the (finality-adjusted) latest number is not higher than the last one you scraped.

Let's discuss. 2 sounds like it has nothing to do with finality and we arbitrarily decided to use the finality as the threshold to determine if this is an error

[guy-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ShahakShama)

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 196 at r1 (raw file):

Previously, ShahakShama wrote…

Let's discuss. 2 sounds like it has nothing to do with finality and we arbitrarily decided to use the finality as the threshold to determine if this is an error

If you prefer that we rename this error LatestBlockNumberTooLow instead, that's fine by me.

[ShahakShama]

@ShahakShama reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware)

---

crates/apollo_l1_provider/src/l1_scraper_tests.rs line 196 at r1 (raw file):

Previously, guy-starkware wrote…

If you prefer that we rename this error LatestBlockNumberTooLow instead, that's fine by me.

Yes, in future PR

[ShahakShama]

@ShahakShama reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)