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: Don't prematurely execute time-triggers before their start timestamp #4333

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Arjentix
Copy link
Contributor

Description

While original issues doesn't state it clearly I think that the problem was premature execution of time triggers.
So that trigger could be executed even before start. I fixed this.

  • Now it's forbidden to register schedule-based time-trigger with start point before latest_block_timestamp + consensus_estimation time, where consensus_estimation = 4 secs and is a hardcoded constant. Everithing before this timestamp is considered already analyzed by Iroha. In other words -- time-triggers in the past are forbidden.
  • As a consequence it's forbidden to register such trigger in genesis
  • Now premature time-trigger look up is forced to not to execute trigger start point is in future
  • This behavior is tested by a fixed change_asset_metadata_after_1_sec test

Forbidding time-triggers in the past was the simpliest solution and I actually think it was making sence to implement that in the initial solution.

Linked issue

Benefits

  • Less confusion about time-trigger execution time
  • No more painful triggers in the past

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@Arjentix Arjentix added Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST labels Feb 27, 2024
@Arjentix Arjentix self-assigned this Feb 27, 2024
@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Feb 27, 2024
@Arjentix Arjentix changed the title [fix] #3262: Don't prematurely execute time-triggers before their start timestamp` [fix] #3262: Don't prematurely execute time-triggers before their start timestamp Feb 27, 2024
@Arjentix Arjentix force-pushed the fix_time_trigger_first_execution_time branch from ff6b8ac to 160c838 Compare February 27, 2024 19:55
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 8070574387

Details

  • -16 of 20 (20.0%) changed or added relevant lines in 3 files are covered.
  • 3125 unchanged lines in 56 files lost coverage.
  • Overall coverage decreased (-0.3%) to 56.519%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core/src/smartcontracts/isi/triggers/mod.rs 0 2 0.0%
data_model/src/isi.rs 0 2 0.0%
data_model/src/events/time.rs 4 16 25.0%
Files with Coverage Reduction New Missed Lines %
core/src/sumeragi/network_topology.rs 1 98.78%
data_model/src/events/time.rs 1 67.81%
tools/kagami/src/crypto.rs 1 12.12%
data_model/src/metadata.rs 2 78.87%
primitives/src/unique_vec.rs 2 91.24%
config/src/kura.rs 3 80.0%
config/src/logger.rs 5 72.73%
logger/src/lib.rs 5 94.12%
tools/kagami/src/main.rs 5 0.0%
crypto/src/signature/secp256k1.rs 7 96.09%
Totals Coverage Status
Change from base Build 7884695009: -0.3%
Covered Lines: 22455
Relevant Lines: 39730

💛 - Coveralls

@s8sato s8sato self-assigned this Feb 28, 2024
@VAmuzing VAmuzing self-assigned this Feb 29, 2024
@mversic mversic assigned mversic and unassigned Arjentix Mar 11, 2024
@nxsaken nxsaken changed the base branch from iroha2-dev to main April 16, 2024 08:31
@nxsaken nxsaken requested a review from dima74 as a code owner April 16, 2024 08:31
@Erigara Erigara self-assigned this May 7, 2024
@Erigara Erigara force-pushed the fix_time_trigger_first_execution_time branch from 160c838 to 5465b6a Compare May 13, 2024 12:32
@Erigara Erigara changed the title [fix] #3262: Don't prematurely execute time-triggers before their start timestamp fix: Don't prematurely execute time-triggers before their start timestamp May 13, 2024
data_model/src/events/time.rs Outdated Show resolved Hide resolved
Comment on lines 131 to 137
test_client.submit_blocking(register_trigger)?;

let value = test_client.request(FindAssetDefinitionKeyValueByIdAndKey {
id: asset_definition_id,
key,
let after_registration_quantity = test_client.request(FindAssetQuantityById {
id: asset_id.clone(),
})?;
assert_eq!(value, numeric!(3).into());
// Schedule start was in the future so trigger isn't executed
assert_eq!(init_quantity, after_registration_quantity);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To conclude that the trigger execution was filtered out, another block than one for registering trigger would need to be committed

data_model/src/events/time.rs Show resolved Hide resolved
…estamp

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
@Erigara Erigara force-pushed the fix_time_trigger_first_execution_time branch from 5465b6a to e03ee2e Compare May 15, 2024 11:49
@mversic mversic requested a review from s8sato May 16, 2024 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[suggestion] Time trigger first execution time
6 participants