Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deferred credits #3229

Merged
merged 8 commits into from
Nov 23, 2022
Merged

Fix deferred credits #3229

merged 8 commits into from
Nov 23, 2022

Conversation

Eitu33
Copy link
Contributor

@Eitu33 Eitu33 commented Nov 21, 2022

  • document all added functions
  • try in sandbox / simulation / labnet
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems

@Eitu33 Eitu33 changed the base branch from main to testnet_17 November 21, 2022 16:43
@AurelienFT
Copy link
Contributor

AurelienFT commented Nov 22, 2022

@Eitu33 Can you add some description on the changes and why so that we can review ?

This was referenced Nov 22, 2022
@Eitu33
Copy link
Contributor Author

Eitu33 commented Nov 22, 2022

@AurelienFT @sydhds What this PR aims to fix:

  1. Deferred credits changes were build to be absolute changes, meaning that the apply of speculative changes to the final deferred credits must override the values for any given existing entry. This is not what was done, the previous behaviour was accumulating the credits what would result in credits slashes never being applying because the change value was set to 0 and that 0 was later summed to the final value instead of replacing it.
  2. The execution of deferred credits only credited the coins equivalent to the given address without setting the credit values to 0, what is even worst is that the only pruning operation we perform on credits removes only the credits that reached 0. The ram was increasing indefinitely because of that.
  3. The previous deferred credits bootstrap didn't take into consideration that credits preceding the execution slot could change, with the denunciations coming they can, so the credits had to parallelly stream its changes in case some happened during the main credits bootstrap process.

@Eitu33 Eitu33 requested review from AurelienFT and sydhds and removed request for AurelienFT November 22, 2022 17:40
@Eitu33 Eitu33 merged commit 0131f4e into testnet_17 Nov 23, 2022
bors bot added a commit that referenced this pull request Nov 30, 2022
3181: Testnet 17 r=AurelienFT a=gterzian

This PR is a batch for testnet 17. These PRs should be merged before merging this one : 

- #3202 
- #3205
- #3162
- #3203
- #3232
- #3192
- #3229 
- #3221 
- #3219 
- #3210 
- #3191 
- #3173 
- massalabs/massa-sc-runtime#161
- massalabs/massa-sc-runtime#164
- #3253 
- #3249 
- #3238 

After merging this PR we should merge these before release testnet 17 : 
- massalabs/massa-as-sdk#49
- massalabs/massa-as-sdk#39

Co-authored-by: AurelienFT <aurelien.foucault@epitech.eu>
@AurelienFT AurelienFT deleted the fix-deferred-credits branch May 29, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants