refactor state #331
refactor state #331
Conversation
remove padding when encoding governance addr
change statedb variables to private
fix some tests in vm_dummy
| @@ -26,8 +26,7 @@ func executeGovernanceTx(ccc consensus.ChainConsensusCluster, bs *state.BlockSta | |||
| } | |||
|
|
|||
| governance := string(txBody.Recipient) | |||
|
|
|||
| scs, err := state.OpenContractState(receiver.AccountID(), receiver.State(), bs.StateDB) | |||
| scs, err := statedb.OpenContractState(receiver.IDNoPadding(), receiver.State(), bs.StateDB) | |||
There was a problem hiding this comment.
Unlike other IDs, the governance ID is saved in state without padding.
contract/vm.go
Outdated
| len(v.curState.GetCodeHash()) != 0 { | ||
| ctx.bs.RemoveCache(id) | ||
| } | ||
| for _, v := range ctx.callState { |
There was a problem hiding this comment.
Can you explain the above removal?
There was a problem hiding this comment.
It appears to mean: if there was not a contract on that address and now there is one, remove the contract code from cache (revert the deploy)
There was a problem hiding this comment.
it`s my miss. i will recover that.
There was a problem hiding this comment.
this logic is recovered using isDeploy flag.
| if err != nil { | ||
| return newDbSystemError(err) | ||
| } | ||
| } | ||
| /* For Sender */ | ||
| if v.prevState == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
this logic is ignore putState if state not changed.
I don`t understand why vm.go have their own prevState and curState, so I remove it.
if it needed to ignore PutState about not updated state, it can be added in state.PutState()
There was a problem hiding this comment.
prevState exists for 2 cases: the one above (to avoid unnecessary disk I/O) and the other about revert deploy (remove code from cache)
I am not sure if it is good to remove it
If removed, it must be replaced by equivalent logic
There was a problem hiding this comment.
you are right. it need to replaced by equivalent logic. it also need to change.
There was a problem hiding this comment.
maybe it is good that all AccountState should be exist in state.blockState, and vm can get/put accountState only using blockState.
- vm commit ( per transaction ) -> change accountState in BlockState
- blockState commit ( per block ) -> change state in db
There was a problem hiding this comment.
this logic is recovered using isCallback flag.
| @@ -68,35 +53,39 @@ func (cs *ContractState) GetAccountID() types.AccountID { | |||
| return cs.account | |||
| } | |||
|
|
|||
| func (cs *ContractState) GetID() []byte { | |||
| return cs.id | |||
There was a problem hiding this comment.
Why this function is named GetID and on AccountState it is just ID?
There was a problem hiding this comment.
because other function in ContractState is GetXXX, It is not easy to remove Get from all other GetXXX functions in contractState.
There was a problem hiding this comment.
if it needed, AccountState functions can be added Get prefix too.
it need to handle error message (C.Cstring)
check isDeploy and isSend
put account state only for callback
|
OK, a sync test can be run to check its backwards compatibility |
remove ClearAid()
|
sync start |
|
sync success |
|
@kroggen can you approve this pr? |
not affect for sync test ( not used function )
|
I will check for conflicts, tomorrow |
contract/vm.go
Outdated
| func (ce *executor) vmLoadCall() { | ||
| if cErrMsg := C.vm_loadcall( | ||
| ce.L, | ||
| ); cErrMsg != nil { |
There was a problem hiding this comment.
On this case the argument can be on the same line as the function
There was a problem hiding this comment.
Why not separate the call from the if clause? The same for the function above
There was a problem hiding this comment.
Why not separate the call from the
ifclause? The same for the function above
it can limiting error variable ( cErrMsg ) within if scope. ( and also shorten line. )
|
@gokch Why I am also curious why many functions were moved from |
|
The separation of |
contractState is only modified by lua contract, but accountState is modified by lua contract and external chain. and, Although it is not confirmed, this also considering evm support. evm and lua have their own state db, but they share same accountState. |
yes, and it is also good to separate vmContext and executor. ( make new file vm_executor.go ) |
What is changed?
add id field in ContractState ( to identify AccountState in vm )
move ContractState to statedb ( it related with evm_state_wip )
change balance modify logic in vm.go to use AccountState (luaSendAmount, luaCallContract, luaDeployContract)
replace add/sub balance logic to state.SendBalance()
fix some tests in vm and governance
Is there risk of fork?
yes. it need to sync test.