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

Simplify get_block_hash() method by returning null hash #155

Merged
merged 1 commit into from
Oct 19, 2018

Conversation

chfast
Copy link
Member

@chfast chfast commented Sep 9, 2018

Continuation of changes in #154.

Reverts #73.

@chfast
Copy link
Member Author

chfast commented Sep 11, 2018

@gumb0 @axic, this one is up to discussion. For me both variants are ok. We can treat this as an experiment.

@gumb0
Copy link
Member

gumb0 commented Sep 12, 2018

I'm fine with this, I think it's nice to be consistent with functions in #154 and it might simplify calling code in VM.

@@ -41,7 +41,7 @@ static inline void go_exported_functions_type_checks()
{
struct evmc_context* context = NULL;
evmc_address* address = NULL;
evmc_bytes32* bytes32 = NULL;
evmc_bytes32 bytes32;
Copy link
Member

Choose a reason for hiding this comment

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

These names bytes32 & uint256be look too much as type names, it confuses me a bit every time.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the intention. I have to do hard-core function pointer casts in Go, because cgo (Go's C tool) does not support const types. Then whenever I change the EVMC part there is no error in Go bindings.

In this "fake" function I'm using the function pointer before case and the one after with exactly the same arguments and return types so this detect almost all mismatches (because non-const types bind to const ones).

docs/EVMC.md Outdated
@@ -40,6 +40,7 @@ can be referenced as EVMC ABIv3 or just EVMC 3.
2. **Host** – An entity controlling the VM.
The Host requests code execution and responses to VM queries by callback
functions. This usually represents an Ethereum Client.
3. **null hash** – a hash which all bytes are zeros.
Copy link
Member

Choose a reason for hiding this comment

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

"for which", "in which" maybe? Or maybe just call it "null bytes" for get_block_hash, too

* a given block. If the requested block is not found, then an appropriate
* result code is returned.
* This callback function is used by a VM to query the hash of the header of the given block.
* If the information about the requested block is not available then this is signaled by
Copy link
Member

Choose a reason for hiding this comment

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

"signalled" and comma before "then"

@chfast
Copy link
Member Author

chfast commented Sep 17, 2018

This has been rebased. Any comments about the design?

@chfast
Copy link
Member Author

chfast commented Oct 15, 2018

Use cases

  1. Contract asks about "recent" block hash (default use case). The VM can pass the request to the Host safely and put the result on the stack.
  2. Contract asks about a future block hash. The VM can pass the request to the Host safely and will get the null hash and/or an error.
  3. Contract asks about an old block hash. The VM cannot pass the request to the Host because the block might be available but BLOCKHASH instructions should put null hash on the stack anyway.

Possible VM implementations

  1. The VM checks if the requested block is in the safe range (256 recent blocks). Only then asks the Host. VM does not expect to get the error / null hash in return.
  1. The VM asks the Host first. When error is received it puts null hash on the stack. If the requests is successful it check it that's not an old block and nulls it if it is.
    This variant at least does requests when they are not needed.

Conclusions

  • The first implementation is better.
  • In the first implementation is slightly better when the Host returns null hash instead of an error.
// Variant with error:
bytes32 hash;
bool success = host->get_block_hash(&hash, number);
assert(success);
if (!success)
    hash = {};  // Make is deterministic.
stack[0] = hash;
// Variant with null hash:
bytes32 hash = host->get_block_hash(number);
assert(hash != {});
stack[0] = hash;

@chfast chfast merged commit 7aedcd0 into master Oct 19, 2018
@chfast chfast deleted the simplify-blockhash branch October 19, 2018 16:28
@axic
Copy link
Member

axic commented Oct 22, 2018

I'm still strictly against this change. Would have preferred not merging it.

@axic
Copy link
Member

axic commented Oct 22, 2018

Contract asks about an old block hash. The VM cannot pass the request to the Host because the block might be available but BLOCKHASH instructions should put null hash on the stack anyway.

This applies to EVM, but nothing else. The main point of the #73 change was that hera wouldn't need to do black magic (checking if get_block_hash returns the null hash) to exhibit special behaviour.

I think this change makes sense if EVMC guarantees that in case of an error the null hash is returned. Which it currently doesn't do.

@chfast
Copy link
Member Author

chfast commented Oct 22, 2018

in case of an error

What kind of error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants