staking: paranoid writeback changes #4466
Conversation
pro-wh
requested review from
kostko,
peterjgilbert,
ptrus and
Yawning
as code owners
| @@ -79,15 +79,14 @@ func (app *stakingApplication) transfer(ctx *api.Context, state *stakingState.Mu | |||
| return nil, err | |||
| } | |||
|
|
|||
| if err = state.SetAccount(ctx, fromAddr, from); err != nil { | |||
| return nil, fmt.Errorf("failed to fetch account: %w", err) | |||
There was a problem hiding this comment.
While you're here, probably change this?
| // copies of it and overwrite it later. | ||
| var from *staking.Account | ||
| if toAddr.Equal(reclaim.Account) { | ||
| from = to |
There was a problem hiding this comment.
This was deliberate if somewhat wonky, but to is never mutated, so the removal appears to be ok.
There was a problem hiding this comment.
right, to prevent creating multiple in-memory representations of the same record that can get out of sync. in the changed version, we no longer attempt to load the "to"/delegator account. to is gone altogether, so we thus still avoid that condition
There was a problem hiding this comment.
The thinking behind the current setup is that these Set calls can only fail in case there is state corruption in which case all state updates are rolled back and consensus halts.
You could also use:
ctx = ctx.NewTransaction()
defer ctx.Close()
// ...
ctx.Commit()
|
oh the transaction way would be nice. this being something that came up while thinking about adding a minimum balance check, it seems more correct to create a transaction and roll back if the minimum balance is not met |
pro-wh
force-pushed
the
pro-wh/feature/writeback
branch
2 times, most recently
from
fb277d9 to
a46e0e9
Compare
|
ok Set* rearrangements removed, NewTransaction + Commit added |
pro-wh
force-pushed
the
pro-wh/feature/writeback
branch
from
a46e0e9 to
a679055
Compare
|
no, this is all wrong, we can't just start a transaction and keep using the same |
Codecov Report
@@ Coverage Diff @@
## master #4466 +/- ##
==========================================
+ Coverage 68.83% 69.06% +0.23%
==========================================
Files 415 415
Lines 46799 46816 +17
==========================================
+ Hits 32214 32334 +120
+ Misses 10625 10543 -82
+ Partials 3960 3939 -21
Continue to review full report at Codecov.
|
transfer/withdraw saves from account first, then to account:
reclaim escrow inconsistencies in partial failure, under new ordering:
add escrow seems fine as is:
do we want these changes? I can split these up if we want some and not others