do not apply transaction optimizations on light-nodes when speculating #5415
Conversation
fixed an issue where block status was not taken into account when determining whether or not to skip some expensive checks. As a result light validation nodes would not apply those checks even when speculatively executing new transactions (which are not signed by producers). Also refactored common logic while maintaining the 2 config/parameters that control different sets of optimizations resolves #5408
| && !my->conf.force_all_checks | ||
| && !my->in_trx_requiring_checks; | ||
| // in a pending irreversible or previously validated block and we have forcing all checks | ||
| bool consider_skipping_on_replay = (pb_status == block_status::irreversible || pb_status == block_status::validated) && !replay_opts_disabled_by_policy; |
There was a problem hiding this comment.
controller has replaying flag. Seems like conf.disable_replay_optsshould be tied to it as block_status::validated could happen on switching forks.
There was a problem hiding this comment.
disable replay opts was added around the same time that light validation was and it intentionally covers replays due to forks as those are also validated nodes.
libraries/chain/controller.cpp
Outdated
| return consider_skipping | ||
| && !my->conf.disable_replay_opts | ||
| && !my->in_trx_requiring_checks; | ||
| light_validation_allowed(my->conf.disable_replay_opts); | ||
| } |
There was a problem hiding this comment.
Seems like we should not allow:
force-all-checks = true
and
disable-replay-ops = true
in chain_plugin.cpp
There was a problem hiding this comment.
For historical reasons they cover disparate sets of optimizations. When we put the feature in, we decided that we would maintain the difference. We shouldn't change those configuration expectations on a patch release, however, I agree that in a future version we may want to make force-all-checks authoritative over disable-replay-opts
|
|
||
|
|
||
| bool controller::skip_auth_check() const { | ||
| return light_validation_allowed(my->conf.force_all_checks); |
There was a problem hiding this comment.
Seems like force-all-checks should override light validation mode and override disable-replay-opts or better yet don't allow both to be specified.
There was a problem hiding this comment.
this is for historical reasons, "force-all-checks" was something specifically tied to replay just replay. The last feature release did bend this a little bit to include the same set of features for replays-due-to forks.
fixed an issue where block status was not taken into account when determining whether or not to skip some expensive checks. As a result light validation nodes would not apply those checks even when speculatively executing new transactions (which are not signed by producers).
Also refactored common logic while maintaining the 2 config/parameters that control different sets of optimizations
resolves #5408