apollo_l1_provider: improve add_tx and prevent double scraping

[guy-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[guy-starkware]

• apollo_l1_provider: replace bootstrapping with catching up #10726
• apollo_l1_provider: rename catch_up_height and historic height #10725
• apollo_l1_provider: scraper historic height is one more #10724
• apollo_l1_provider: minor updates to some bootstrapping tests of the L1 provider #10702
• apollo_l1_provider: add reset_bootstrapper function #10513
• apollo_l1_provider: check for uninitialized instead of bootstrapping+sync started #10512
• apollo_l1_provider: add verification that tx is pending when getting it from proposable index #10511
• apollo_l1_provider: improve add_tx and prevent double scraping #10508 👈 (View in Graphite)
• apollo_l1_provider: add l1_block_time as a config parameter #10505
• apollo_l1_provider: change L1_BLOCK_TIME to be 12 seconds #10544
• apollo_l1_provider: fix log message when skipping over historic heights #10465
• apollo_l1_provider: reduce number of provider error variants #10414
• apollo_l1_provider: remove some panics in the L1 Provider #10411
• main-v0.14.1

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

[sirandreww-starkware]

@sirandreww-starkware reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @guy-starkware)

---

crates/apollo_l1_provider/src/transaction_manager.rs line 179 at r1 (raw file):

                    );
                }
                record.tx.set(tx, block_timestamp, scrape_timestamp);

You set either way? should there be a check that hashes match?

[guy-starkware]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @sirandreww-starkware)

---

crates/apollo_l1_provider/src/transaction_manager.rs line 179 at r1 (raw file):

Previously, sirandreww-starkware (Andrew Luka) wrote…

You set either way? should there be a check that hashes match?

The record is stored by hash. You can't have a duplicated tx if the hash doesn't match (it will be recorded as a new tx).

[guy-starkware]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @sirandreww-starkware)

---

crates/apollo_l1_provider/src/transaction_manager.rs line 179 at r1 (raw file):

Previously, guy-starkware wrote…

The record is stored by hash. You can't have a duplicated tx if the hash doesn't match (it will be recorded as a new tx).

The reason we set it even if it exists is that you can get a HashOnly payload from sync, and then get it again from the scraper, which sets it to a Full payload.

I'm adding a situation where it was already a Full payload and we try to set it again, which is a bug. This shouldn't happen but if it does we don't want to accidentally re-put it on the proposable index (the queue we are using to validate or propose).

[sirandreww-starkware]

Reviewable status: 1 of 3 files reviewed, all discussions resolved

---

crates/apollo_l1_provider/src/transaction_manager.rs line 179 at r1 (raw file):

Previously, guy-starkware wrote…

The reason we set it even if it exists is that you can get a HashOnly payload from sync, and then get it again from the scraper, which sets it to a Full payload.

I'm adding a situation where it was already a Full payload and we try to set it again, which is a bug. This shouldn't happen but if it does we don't want to accidentally re-put it on the proposable index (the queue we are using to validate or propose).

I see

[ShahakShama]

@ShahakShama reviewed 1 of 3 files at r1, 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/transaction_manager.rs line 168 at r2 (raw file):

        // If exists, return false and do nothing. If not, create the record as a HashOnly payload.
        let is_new_record = self.create_record_if_not_exist(tx_hash);
        // Replace a HashOnly payload with a Full payload. If a full payload already exists, do not

Describe the entire thing in one sentence. then explain why in another sentence
Replacing a HashOnly payload is allowed, but a full payload is forbidden. The reason why is that a double scrape should not happen and may cause the tx to be re-added to the proposable index

---

crates/apollo_l1_provider/src/transaction_manager.rs line 176 at r2 (raw file):

                    info!(
                        "Transaction {tx_hash} already exists as a HashOnly payload. It was \
                         probably gotten via sync, and is now updated with a Full payload."

the word sync is ambiguous. Try to add more words to describe which sync
consider "via bootstrap from sync" or "via state sync component"

[guy-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)

---

crates/apollo_l1_provider/src/transaction_manager.rs line 168 at r2 (raw file):

Previously, ShahakShama wrote…

Describe the entire thing in one sentence. then explain why in another sentence
Replacing a HashOnly payload is allowed, but a full payload is forbidden. The reason why is that a double scrape should not happen and may cause the tx to be re-added to the proposable index

Done.

---

crates/apollo_l1_provider/src/transaction_manager.rs line 176 at r2 (raw file):

Previously, ShahakShama wrote…

the word sync is ambiguous. Try to add more words to describe which sync
consider "via bootstrap from sync" or "via state sync component"

Done.

[guy-starkware]

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

[guy-starkware]

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