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

feat!: introduce block header into every smart contract execution context #5151

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

mversic
Copy link
Contributor

@mversic mversic commented Oct 16, 2024

closes #4958

Block header is provided as context of execution for:

  1. migrate payload (as current block header)
  2. execute_instruction (as current block header)
  3. execute_transaction (as current block header)
  4. validate_query (as latest committed block header)
  5. smart contract and trigger execution (as current block header)

As a consequence of this change block header variable was added to StateBlock and StateTransaction. While doing this I get the feeling that StateBlock could take ownership of the whole block that it is validating

@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Oct 16, 2024
@mversic mversic force-pushed the wasm_block_header_context branch 7 times, most recently from 9c8f8eb to 3437e43 Compare October 17, 2024 09:29
@mversic mversic requested a review from s8sato October 18, 2024 04:27
@mversic mversic force-pushed the wasm_block_header_context branch from 3437e43 to 3599869 Compare October 18, 2024 04:27
@mversic mversic enabled auto-merge (squash) October 18, 2024 04:27
@mversic mversic force-pushed the wasm_block_header_context branch 4 times, most recently from 7c464b8 to 2daddf4 Compare October 18, 2024 08:58
@mversic mversic force-pushed the wasm_block_header_context branch from 2daddf4 to 0b133cb Compare October 18, 2024 12:52
@mversic mversic force-pushed the wasm_block_header_context branch from 0b133cb to ce502ac Compare October 18, 2024 12:52
Comment on lines 960 to +963
fn get_smart_contract_context(state: &state::SmartContract) -> payloads::SmartContractContext {
payloads::SmartContractContext {
authority: state.authority.clone(),
curr_block: state.state.0.curr_block,
Copy link
Contributor

@s8sato s8sato Oct 18, 2024

Choose a reason for hiding this comment

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

  1. smart contract and trigger execution (as latest current block header)

There seems to be an error on documentation side:
Since the execution phase of a smartcontract (wasm as a transaction executable) is different from that of a trigger, each pair (block height, what executions refer the block header) is as follows:

>>>>>>>>>> block committed >>>>>>>>>> triggers executed
current: StateTransaction.curr_block k-1 or k, transaction k, trigger
latest committed: CommonState.specific_state.curr_block k-1, query k, query?

So a smartcontract refers the current (validating) block header, not the latest one. On the other hand, it is true that a block header that a trigger refers is current and latest committed

EDIT:
I'm not sure about queries during a trigger execution. At least during a by-call trigger execution, it seems ExecuteTransaction state blindly lets a query pass. So far this issue hasn't surfaced because even if it were connected to visit_query, that would validate nothing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. But this is only because our trigger execution model is broken

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, when transaction and trigger execution are integrated and sequenced, such a discussion would be unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this should be fixed for queries that are executing as part of a transaction (like from inside a trigger). They should receive current block header instead of latest block header. Do you agree?

Any other issues to address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened #5171

@mversic mversic merged commit 07939d1 into hyperledger-iroha:main Oct 18, 2024
15 checks passed
@mversic mversic deleted the wasm_block_header_context branch October 20, 2024 20:31
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide current block header to smart contracts
3 participants