Dont skip eager rent collect across gapped epochs #10206
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #10206 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 296 296
Lines 69320 69414 +94
=======================================
+ Hits 56622 56710 +88
- Misses 12698 12704 +6 |
ryoqun
commented
May 24, 2020
ryoqun
commented
May 24, 2020
ryoqun
commented
May 25, 2020
ryoqun
commented
May 25, 2020
|
|
||
| fn map_to_test_bad_range() -> AccountMap<Pubkey, i8> { | ||
| let mut map: AccountMap<Pubkey, i8> = AccountMap::new(); | ||
| // when empty, AccountMap (= std::collections::BTreeMap) doesn't sanitize given range... |
There was a problem hiding this comment.
I'm planning to create a small pr to rustc.
|
@sakridge Could you review this not as draft so that I can merge this and back-port this to v1.2 for devnet and testnet(tds)? |
|
For mainnet-beta (assuming the new 1.1 branch), I'll back-port this after epoch 34 (at which the eager rent collection activates). |
ryoqun
commented
May 26, 2020
|
@ryoqun - Think we can avoid backporting this to v1.1? |
|
@mvines Yeah, but we nevertheless need gating logics in the |
|
Ok, do you think the process I outlined at https://discordapp.com/channels/428295358100013066/489504501749907468/715680679379075073 earlier today would work here? |
Yeah, that definitely works! |
ryoqun
commented
May 29, 2020
ryoqun
commented
May 29, 2020
| partitions.push(( | ||
| parent_cycle_index, | ||
| parent_last_cycle_index, | ||
| slot_count_in_two_day, | ||
| )); | ||
|
|
||
| // ... for current cycle | ||
| partitions.push((0, 0, slot_count_in_two_day)); |
There was a problem hiding this comment.
This fn (fixed_cycle_partitions) is mostly for benches, so doesn't need any gating like this: https://github.com/solana-labs/solana/pull/10206/files#diff-a7549f152920d85fb44e6a784b8e5e1fR1830
sakridge
reviewed
Jun 2, 2020
runtime/src/bank.rs
Outdated
| @@ -1799,6 +1850,7 @@ impl Bank { | |||
| start_slot_index: SlotIndex, | |||
| end_slot_index: SlotIndex, | |||
| epoch: Epoch, | |||
| auto_generated: bool, | |||
There was a problem hiding this comment.
what does it mean exactly? Do we always pass true?
There was a problem hiding this comment.
Well, the naming was bad... I think I've made the intention clearer by some code changes and commenting..
This can be true/false in fact. This indicates given slot indexes are somewhat special ones or not. Namely, from this code block: https://github.com/solana-labs/solana/pull/10206/files#diff-a7549f152920d85fb44e6a784b8e5e1fR1843-R1869. And this corresponds to this in the ascii table.
sakridge
reviewed
Jun 2, 2020
sakridge
previously approved these changes
Jun 3, 2020
ryoqun
force-pushed
the
skipped-eager-rent-collection
branch
from
5c107ae to
e1f1cb3
Compare
ryoqun
commented
Jun 9, 2020
| #[cfg(not(test))] | ||
| let should_enable = match self.operating_mode() { | ||
| OperatingMode::Development => true, | ||
| OperatingMode::Preview => current_epoch >= Epoch::max_value(), |
mvines
reviewed
Jun 9, 2020
|
I'm planning to land this next week. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
stale
bot
added
stale
|
It seems that testnet got a lot less forky... (I tried to repro this edge case on epoch 60 -> 61 at the testnet, but that didn't occur...) So, I've cherry-picked this onto v1.2 and replayed the past skipped epoch boundary around the slots described in this issue description. As seen below, now correct range is inserted: |
|
And it started to emit |
mergify bot
pushed a commit
that referenced
this pull request
Jun 25, 2020
* Dont skip eager rent collect across gapped epochs * Adjust style and comment * Adjust ascii chart and comment a bit * Moar assert * Relax the partition_count assert for completeness * Tweak comment... * tweak a bit * Add gating logic * Address reviews * small formatting * Clarify the code by replacing auto_generated... * small formatting * small formatting * small formatting * small formatting * Narrow down conditional compilation scope (cherry picked from commit 50f7ed8)
This was referenced Jun 25, 2020
mergify bot
added a commit
that referenced
this pull request
Jun 25, 2020
* Dont skip eager rent collect across gapped epochs * Adjust style and comment * Adjust ascii chart and comment a bit * Moar assert * Relax the partition_count assert for completeness * Tweak comment... * tweak a bit * Add gating logic * Address reviews * small formatting * Clarify the code by replacing auto_generated... * small formatting * small formatting * small formatting * small formatting * Narrow down conditional compilation scope (cherry picked from commit 50f7ed8) Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
2 tasks
ryoqun
added a commit
to ryoqun/solana
that referenced
this pull request
Jun 26, 2020
* Dont skip eager rent collect across gapped epochs * Adjust style and comment * Adjust ascii chart and comment a bit * Moar assert * Relax the partition_count assert for completeness * Tweak comment... * tweak a bit * Add gating logic * Address reviews * small formatting * Clarify the code by replacing auto_generated... * small formatting * small formatting * small formatting * small formatting * Narrow down conditional compilation scope
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
eager rent collection can be skipped when a new epoch begins from a parent bank with gaps in slots:
This is tds:
It should have
(0-0)/432000. The current code only have it correctly when there is no gap across epochs. Note that the pubkey range is missing[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, ..].This doesn't introduce undeterministic. It only doesn't eager-collect some pubkeys near
[00, 00, 00, 00, ..Summary of Changes
Correctly cover the missing range at slot index 0 when new epoch is gapped.
follow up #9527
I'm ...
sad. ;) This also means we need to introduce this with gated fashion for tds. For mainnet-beta, we just need to apply this updated eager rent collection straightly.