statemanager: refactor logic into capabilities

[gabrocheleau]

This PR refactors some duplicate logic from statemanager classes into a separate capabilities helper

[codecov]

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 41.31%. Comparing base (fb50628) to head (12b45ba).
Report is 17 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
block	73.57% <ø> (?)
blockchain	85.65% <ø> (?)
client	?
common	89.26% <ø> (?)
devp2p	0.00% <ø> (?)
evm	63.52% <ø> (?)
genesis	0.00% <ø> (?)
statemanager	65.75% <95.23%> (?)
trie	51.96% <ø> (?)
tx	?
util	69.89% <ø> (?)
vm	58.49% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

[acolytec3]

DRY! LGTM!

[holgerd77]

Ah, TBH: I do not like this yet, this structural accessing on private properties. 🤔

Will give this some thought.

[holgerd77]

These Common interfaces should really only be used for the "3 big defining interfaces" (like StateManager) which are important beyond the scope of a single package and therefore should get some solid visibility and the possibility to re-use.

(so generally it's already too crowded in here and some interfaces do not really belong here)

So these Cache interfaces should definitely not be placed here and should go back if it's not absolutely structurally necessary (I guess it's not?).

[holgerd77]

(maybe this should be stated better at the top of this file)

[holgerd77]

Ok, so I am not satisfied with this structure yet and I think we should continue to evolve.

So, this capabilities concept holds for the modfiyAccountFields() method (not sure if we should still call this Capabilities for this one single method or a bit overblown, but rather secondary question), but not for the caches.

Maybe we do a separate wrapping Caches class which holds all the three caches together (maybe you meantioned this as well in the chat?) (also the settings should the fully move over there), which can then be initialized separately?

This would abstract all this a lot better away (then maybe though forget my comment on Common interface inclusion, guess this is still needed then (or maybe not, and we handle this more internally in the StateManager package? So this would definitely be prefered!)).

Then a StateManager would have a property caches (not sure if needed to be public or not), and this would have (public) properties for the account/code/storage cache so that one could do this.caches.account.delete() from within the SMs.

I think we could then also make a lot of code like this a lot more atomic:

/**
   * Deletes an account from state under the provided `address`.
   * @param address - Address of the account which should be deleted
   */
  async deleteAccount(address: Address) {
    if (this.DEBUG) {
      this._debug(`Delete account ${address}`)
    }

    this._codeCache?.del(address)

    if (this._accountCacheSettings.deactivate) {
      await this._trie.del(address.bytes)
    } else {
      this._accountCache!.del(address)
    }
    if (!this._storageCacheSettings.deactivate) {
      this._storageCache?.clearStorage(address)
    }
  }

so that this could be more or less replaced with this.caches?.deleteAccount() or so (+ the trie deletion).

I would then also pledge that we make it mandatory to have this Caches object intitialized manually along SM instantiation (if wished to be used), like:

new StateManager({ caches: new Caches() }) 

Then we can tree shake away the whole cache code for non users as a side effect.

For the Capabilities I think there is a lot more room on the other hand to integrate (partial) methods on the account/code/storage side.

So for something like this it should be likely possible to move everything except the first line into a capability method? (guess this is similarly used in another SM? but didn't have a look)

/**
   * Adds `value` to the state trie as code, and sets `codeHash` on the account
   * corresponding to `address` to reference this.
   * @param address - Address of the `account` to add the `code` for
   * @param value - The value of the `code`
   */
  async putCode(address: Address, value: Uint8Array): Promise<void> {
    this._codeCache?.put(address, value)
    const codeHash = this.keccakFunction(value)

    if (this.DEBUG) {
      this._debug(`Update codeHash (-> ${short(codeHash)}) for account ${address}`)
    }

    if ((await this.getAccount(address)) === undefined) {
      await this.putAccount(address, new Account())
    }
    await this.modifyAccountFields(address, { codeHash })
  }

Ok, so far! 🙂

[holgerd77]

Ah, yeah, and the StateManager interface should generally not be bloated in any way, that was one big reason for the refactor of the interface we did 1-2 weeks ago.

This should remain minimal (also regarding optional properties, though it's a bit less urgent here), so that third-party users are not burdened with the internals of state manager.

So all these properties should definitely go out again.

[gabrocheleau]

Ok, so I am not satisfied with this structure yet and I think we should continue to evolve.

So, this capabilities concept holds for the modfiyAccountFields() method (not sure if we should still call this Capabilities for this one single method or a bit overblown, but rather secondary question), but not for the caches.

Maybe we do a separate wrapping Caches class which holds all the three caches together (maybe you meantioned this as well in the chat?) (also the settings should the fully move over there), which can then be initialized separately?

This would abstract all this a lot better away (then maybe though forget my comment on Common interface inclusion, guess this is still needed then (or maybe not, and we handle this more internally in the StateManager package? So this would definitely be prefered!)).

Then a StateManager would have a property caches (not sure if needed to be public or not), and this would have (public) properties for the account/code/storage cache so that one could do this.caches.account.delete() from within the SMs.

I think we could then also make a lot of code like this a lot more atomic:

/**
   * Deletes an account from state under the provided `address`.
   * @param address - Address of the account which should be deleted
   */
  async deleteAccount(address: Address) {
    if (this.DEBUG) {
      this._debug(`Delete account ${address}`)
    }

    this._codeCache?.del(address)

    if (this._accountCacheSettings.deactivate) {
      await this._trie.del(address.bytes)
    } else {
      this._accountCache!.del(address)
    }
    if (!this._storageCacheSettings.deactivate) {
      this._storageCache?.clearStorage(address)
    }
  }

so that this could be more or less replaced with this.caches?.deleteAccount() or so (+ the trie deletion).

I would then also pledge that we make it mandatory to have this Caches object intitialized manually along SM instantiation (if wished to be used), like:

new StateManager({ caches: new Caches() }) 

Then we can tree shake away the whole cache code for non users as a side effect.

For the Capabilities I think there is a lot more room on the other hand to integrate (partial) methods on the account/code/storage side.

So for something like this it should be likely possible to move everything except the first line into a capability method? (guess this is similarly used in another SM? but didn't have a look)

/**
   * Adds `value` to the state trie as code, and sets `codeHash` on the account
   * corresponding to `address` to reference this.
   * @param address - Address of the `account` to add the `code` for
   * @param value - The value of the `code`
   */
  async putCode(address: Address, value: Uint8Array): Promise<void> {
    this._codeCache?.put(address, value)
    const codeHash = this.keccakFunction(value)

    if (this.DEBUG) {
      this._debug(`Update codeHash (-> ${short(codeHash)}) for account ${address}`)
    }

    if ((await this.getAccount(address)) === undefined) {
      await this.putAccount(address, new Account())
    }
    await this.modifyAccountFields(address, { codeHash })
  }

Ok, so far! 🙂

Thanks for the suggestion. Went back and forth on this, wasn't sure how much caching updates we were comfortable doing, as cache can differ from one state manager to the next, will experiment and come back with something :)

[holgerd77]

Thanks for the suggestion. Went back and forth on this, wasn't sure how much caching updates we were comfortable doing, as cache can differ from one state manager to the next, will experiment and come back with something :)

Ah, no, you somewhere got me wrong here. That was no suggestion to change the number of caching updates whatsoever (we definitely should not do that, especially just for a form-refactor)

[holgerd77]

So this is just a suggestion how to re-package stuff a bit.

1. There is a new dedicated class Caches
2. SMs get an internal member (protected) _caches
3. Caches move over there _accountCache -> Caches.account e.g., become public
4. Access goes like this._caches.account
5. Settings move over there (obviously also the associated options)
   (generally - optimally - Caches should be fully encapsuled, so that the initialized SM does not need to be passed over any more on function calls)
6. Init stuff from SM constructors move over there to an init() method
7. All accesses in SMs are adjusted

Optimally we do not expose cache stuff at all in the StateManagerInterface. This is just a thing one (also external implementers of new SMs) can use internally.

For something like the deleteAccount() example (likely others) above we can then move more or less all the code to a Caches method:

Caches {

 deleteAccount(address: Address) {
    this.code.del(address)
    this.account.del(address)
    this.storage.clearStorage(address)
  }
}

And in the main SM only this remains:

async deleteAccount(address: Address) {
    if (this.DEBUG) {
      this._debug(`Delete account ${address}`)
    }
    this._caches?.deleteAccount(address)
    if (this._caches === undefined) { 
      await this._trie.del(address.bytes)
    }
  }

So but these kind of optimizations (if possible) are just somewhat of an extra I guess. The general restructuring would already bring most of the encapsuation benefits.

If you want we can also have a call on this later the (my) day!