Moved deserialization of blobs to entries from replicate_stage to window_service #1287
Conversation
src/window.rs
Outdated
| // window and exit. *k_data_slot cannot be None at this point, | ||
| // so it's safe to unwrap. | ||
| let old = mem::replace(k_data_slot, None).unwrap(); | ||
| recycler.recycle(old, "insert_blob_is_dup"); |
There was a problem hiding this comment.
Can you rebase on #1290 after it's merged?
src/window.rs
Outdated
| // If the blob can't be deserialized, then remove it from the | ||
| // window and exit. *k_data_slot cannot be None at this point, | ||
| // so it's safe to unwrap. | ||
| let old = mem::replace(k_data_slot, None).unwrap(); |
There was a problem hiding this comment.
let old = k_data_slot.take();
carllin
force-pushed
the
MoveEntryDeserialization
branch
2 times, most recently
from
19b1df1 to
8b80cd0
Compare
src/window_service.rs
Outdated
| w.meta.set_addr(&tn.info.contact_info.ncp); | ||
| } | ||
| msgs.push(b); | ||
| let mut recorder = Recorder::new(Hash::default()); |
There was a problem hiding this comment.
This is too much logic to put into a unit test. Can you pull it out into its own function? Not sure if it belongs in this module or useful enough to have a better home? And so much going on there it looks like it could use some tests of its own. Especially the rogue truncate that looks to say, "oops, went too far, but no worries, truncate()."
There was a problem hiding this comment.
Yeah, definitely should extract it into a separate function! The truncate was to decouple us from any assumptions about how many entries the Recorder will produce from an empty vector of transactions. Right now an empty vector of transactions should map to exactly one entry so the truncate is unnecessary, but this may not always be the case.
src/window_service.rs
Outdated
| @@ -144,7 +145,7 @@ fn recv_window( | |||
| consumed: &mut u64, | |||
| received: &mut u64, | |||
| r: &BlobReceiver, | |||
| s: &BlobSender, | |||
| s: &Sender<Vec<Entry>>, | |||
There was a problem hiding this comment.
Following the "When in Rome" philosophy mentioned in CONTRIBUTING.md, you need to either add an EntrySender type alias somewhere or get rid of the BlobReceiver alias. No mixing of coding styles within a single module please.
carllin
force-pushed
the
MoveEntryDeserialization
branch
from
8b80cd0 to
398c9a8
Compare
…olana-labs#1287) Bumps [@solana/web3.js](https://github.com/solana-labs/solana-web3.js) from 0.92.1 to 0.92.2. - [Release notes](https://github.com/solana-labs/solana-web3.js/releases) - [Commits](solana-labs/solana-web3.js@v0.92.1...v0.92.2) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Moving the deserialization of blobs to entries to the window_service so that we don't increment the "consumed" count in the window unless the blob is valid. This prevents cases where the window sends the replicate_stage bogus blobs that it can't deserialize into entries, in which case the blob for that height never get detected/ repaired b/c the window has moved on.