feat(sync): sync pending data

[DvirYo-starkware]

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

Other information

---

This change is 

[DvirYo-starkware]

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @dan-starkware)

---

crates/papyrus_sync/src/sources/pending.rs line 37 at r1 (raw file):

#[cfg_attr(test, automock)]
#[async_trait]
pub trait PendingSourceTrait {

Also, pending class sync will be added to this trait.

[codecov]

Codecov Report

Merging #1175 (da330be) into main (f604edd) will decrease coverage by 0.24%.
The diff coverage is 45.12%.

@@            Coverage Diff             @@
##             main    #1175      +/-   ##
==========================================
- Coverage   71.97%   71.74%   -0.24%     
==========================================
  Files          78       79       +1     
  Lines        7604     7680      +76     
  Branches     7604     7680      +76     
==========================================
+ Hits         5473     5510      +37     
- Misses       1254     1285      +31     
- Partials      877      885       +8     

Files	Coverage Δ
crates/papyrus_sync/src/lib.rs	70.40% <72.72%> (+0.63%)	⬆️
crates/papyrus_node/src/main.rs	2.40% <0.00%> (-0.41%)	⬇️
crates/papyrus_sync/src/sources/pending.rs	20.83% <20.83%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[DvirYo-starkware]

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @dan-starkware)

a discussion (no related file):
If you think we should do the pending sync in a different task, lets talk about this. There are disadvantages but they are not so bad.

---

---

crates/papyrus_sync/src/sources/pending.rs line 45 at r3 (raw file):

    for GenericPendingSource<TStarknetClient>
{
    async fn get_pending_data(&self) -> Result<PendingData, PendingError> {

Maybe I will change the return type to something more convenient to our gateway (currently, the format is what the feeder returns)

[dan-starkware]

Reviewed 6 of 7 files at r1, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @DvirYo-starkware)

---

crates/papyrus_sync/src/lib.rs line 629 at r3 (raw file):

}

#[allow(clippy::too_many_arguments)]

Consider gathering in a single pending argument instead.

Code quote:

#[allow(clippy::too_many_arguments)]

---

crates/papyrus_sync/src/lib.rs line 645 at r3 (raw file):

    try_stream! {
        loop {

Please remove the empty line

---

crates/papyrus_sync/src/lib.rs line 657 at r3 (raw file):

            if header_marker == central_block_marker {
                // Only if the node is fully synced until the last known block (and the chain isn't empty), sync pending data.
                if reader.begin_ro_txn()?.get_state_marker()? == header_marker && header_marker!=BlockNumber::default(){

Do we have this functionality elsewhere?
Assuming this logic is common and going to change, probably better having this as a function

Code quote:

reader.begin_ro_txn()?.get_state_marker()? == header_marker

---

crates/papyrus_sync/src/lib.rs line 887 at r3 (raw file):

            header_marker
                .prev()
                .expect("We start asking for pending data only if the chain isn't empty"),

Can there be a race condition in which a revert happened s.t there is no prev?
Why not returning instead?

Code quote:

.expect("We start asking for pending data only if the chain isn't empty"),

---

crates/papyrus_sync/src/sync_test.rs line 253 at r3 (raw file):

        block.block.parent_block_hash = BlockHash(stark_felt!("0x1"));
        Ok(block)
    });

Please explain this part

Code quote:

    mock_pending_source.expect_get_pending_data().times(1).returning(|| {
        let mut block = PendingData::default();
        block.block.parent_block_hash = BlockHash(stark_felt!("0x1"));
        Ok(block)
    });

[DvirYo-starkware]

Reviewable status: 5 of 7 files reviewed, 6 unresolved discussions (waiting on @dan-starkware)

---

crates/papyrus_sync/src/lib.rs line 629 at r3 (raw file):

Previously, dan-starkware wrote…

Consider gathering in a single pending argument instead.

I added to do. I will know how to deal with that better when I finish the pending class sync.

---

crates/papyrus_sync/src/lib.rs line 645 at r3 (raw file):

Previously, dan-starkware wrote…

Please remove the empty line

Done.

---

crates/papyrus_sync/src/lib.rs line 657 at r3 (raw file):

Previously, dan-starkware wrote…

Do we have this functionality elsewhere?
Assuming this logic is common and going to change, probably better having this as a function

I don't see this functionality elsewhere.

---

crates/papyrus_sync/src/lib.rs line 887 at r3 (raw file):

Previously, dan-starkware wrote…

Can there be a race condition in which a revert happened s.t there is no prev?
Why not returning instead?

All the sync happens on a single thread, so if a revert happens, the sync will restart, and we check again that the chain is empty.

[dan-starkware]

Reviewed 1 of 3 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @DvirYo-starkware)