Move blockhash from block.blockhash to global level. by erak · Pull Request #3646 · argotorg/solidity

erak · 2018-03-05T15:44:08Z

chriseth · 2018-03-05T15:52:38Z

docs/miscellaneous.rst

 - ``assert(bool condition)``: abort execution and revert state changes if condition is ``false`` (use for internal error)
 - ``require(bool condition)``: abort execution and revert state changes if condition is ``false`` (use for malformed input or error in external component)
 - ``revert()``: abort execution and revert state changes
+- ``blockhash(uint blockNumber) returns (bytes32)``: hash of the given block - only works for 256 most recent blocks


I think all these values should be sorted somehow...

Do you have a strong opinion on how they should be sorted? Otherwise I'd probably sort them by variable in alphabetical order.

Yes, alphabetically, but that can be a separate PR.

chriseth · 2018-03-05T15:52:49Z

docs/miscellaneous.rst

 Global Variables
 ================

- ``block.blockhash(uint blockNumber) returns (bytes32)``: hash of the given block - only works for 256 most recent blocks


Please keep the old one and mark it as deprecated.

chriseth · 2018-03-05T15:53:07Z

docs/units-and-global-variables.rst

 Block and Transaction Properties
 --------------------------------

- ``block.blockhash(uint blockNumber) returns (bytes32)``: hash of the given block - only works for 256 most recent blocks excluding current


Were the two leading -- here on purpose?

Did you check if they have any effect?

chriseth · 2018-03-05T15:53:31Z

docs/units-and-global-variables.rst


 .. note::
-    Do not rely on ``block.timestamp``, ``now`` and ``block.blockhash`` as a source of randomness,
+    Do not rely on ``block.timestamp`` and ``now`` as a source of randomness,


Please keep blockhash.

chriseth · 2018-03-05T15:53:37Z

docs/units-and-global-variables.rst

    but the only guarantee is that it will be somewhere between the timestamps of two
    consecutive blocks in the canonical chain.

-.. note::


It probably does not hurt to have all the warnings in both places.

chriseth · 2018-03-05T15:54:13Z

libsolidity/analysis/ViewPureChecker.cpp

 		break;
 	case Type::Category::Magic:
 		// we can ignore the kind of magic and only look at the name of the member
-		if (member != "data" && member != "sig" && member != "blockhash")


Please keep the old one.

chriseth · 2018-03-05T15:54:45Z

libsolidity/ast/Types.cpp

 		return MemberList::MemberMap({
 			{"coinbase", make_shared<IntegerType>(160, IntegerType::Modifier::Address)},
 			{"timestamp", make_shared<IntegerType>(256)},
-			{"blockhash", make_shared<FunctionType>(strings{"uint"}, strings{"bytes32"}, FunctionType::Kind::BlockHash, false, StateMutability::View)},


Please only remove it if pragma experimental "v0.5.0" is set.

chriseth · 2018-03-05T15:55:28Z

test/libsolidity/SolidityEndToEndTest.cpp

+		}
+	)";
+ 	compileAndRun(sourceCode);
+	BOOST_CHECK_EQUAL(callContractFunction("a()").empty(), false);


Please use ABI_CHECK(callContractFunction(...), encodeArgs(0))

Ah sorry, I misunderstood this. I think it is easier to read if you just use
BOOST_CHECK(!callContractFunction("a()").empty()), or perhaps even BOOST_CHECK_EQUAL(callContractFunction("a()").length(), 32);

ekpyron · 2018-03-05T16:20:18Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+	char const* text = R"(
+		contract test {
+			function blockhash() pure public returns (bytes32) {}
+		}


Maybe add another function to the test contract that calls the local blockhash and checks whether the compiler resolves it correctly, e.g. by using a return type that is incompatible to the return type of global blackhash and assigning the result of the function call to a variable?

I think it's better to have a function with identical signature (including return type) and use an EndToEnd test to check.

erak · 2018-03-05T18:34:21Z

Updated

chriseth · 2018-03-05T19:07:00Z

Tests are failing: blockhash_not_available_in_block and blockhash_shadow

chriseth · 2018-03-05T19:14:37Z

Sorry, merged the other one first ;)
Please rebase!

axic · 2018-03-05T20:36:21Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+	char const* text = R"(
+		contract Test {
+			function a() public returns (bytes32) {
+				return block.blockhash(0);


This should only fail if v050 mode is selected.

axic · 2018-03-05T20:36:32Z

test/libsolidity/ViewPureChecker.cpp

 	vector<string> view{
 		"block.coinbase",
 		"block.timestamp",
-		"block.blockhash(7)",


These shouldn't be removed (in v050 mode).

ekpyron · 2018-03-05T23:01:00Z

It may be worth to consider, whether these changes could/should be replaced by a change to the static analyzer instead; similarly to preferring the solution of #3652 over the solution of #3643.

erak · 2018-03-07T09:51:18Z

@ekpyron I will have a look into your suggestion regarding deprecation of block.blockhash().

chriseth · 2018-03-07T11:03:19Z

The list of commits looks a little bit messed up :)

erak · 2018-03-07T11:08:20Z

@chriseth Yep, I accidentally messed it up. Should be fine now :) There's just this one comment about deprecating block.blockhash() left, I have to look into. Will let you know when this PR is final...

erak · 2018-03-07T17:26:20Z

Ready for review.

chriseth · 2018-03-09T12:59:29Z

There are still two consecutive commits with the same description.

chriseth · 2018-03-09T12:59:49Z

docs/miscellaneous.rst

 ================

- ``block.blockhash(uint blockNumber) returns (bytes32)``: hash of the given block - only works for 256 most recent blocks
+- ``block.blockhash(uint blockNumber) returns (bytes32)``: hash of the given block - only works for 256 most recent blocks - deprecated in version 0.4.21 and to be replaced by ``blockhash(uint blockNumber)``.


Please update the version to 0.4.22

erak · 2018-03-09T14:50:09Z

@chriseth I've change the version number mentioned in your last comment and also checked the additional - in the docs: it probably slipped in by accident and can be removed safely since it's ignored while rendering the bullet list.

chriseth · 2018-03-12T13:33:53Z

libsolidity/ast/Types.cpp

-			{"number", make_shared<IntegerType>(256)},
-			{"gaslimit", make_shared<IntegerType>(256)}
-		});
+		{


In case-statements the { usually have the same indentation level as the case keyword itself.

Did you actually change anything here?

No. In the end I did not change anything. These braces are leftovers from a previous commit and I'll remove them.

chriseth

Only the small formatting issue left in my opinion.

erak · 2018-03-12T13:54:51Z

@chriseth Fixed the formatting

chriseth · 2018-03-12T14:42:46Z

@axic please merge if you are fine with this PR.

axic · 2018-03-14T17:35:41Z

Sorting out the rebase.

chriseth · 2018-03-15T14:10:33Z

Tests are passing. Can be merged in my opinion.

axic · 2018-03-21T17:40:47Z

docs/units-and-global-variables.rst

 Block and Transaction Properties
 --------------------------------

- ``block.blockhash(uint blockNumber) returns (bytes32)``: hash of the given block - only works for 256 most recent blocks excluding current


Can't comment inline, but the note below still uses block.blockhash, should be updated, since the copy of that note in miscellaneous does the same.

axic · 2018-03-21T17:42:28Z

docs/miscellaneous.rst

 ================

- ``block.blockhash(uint blockNumber) returns (bytes32)``: hash of the given block - only works for 256 most recent blocks
+- ``block.blockhash(uint blockNumber) returns (bytes32)``: hash of the given block - only works for 256 most recent blocks - deprecated in version 0.4.22 and to be replaced by ``blockhash(uint blockNumber)``.


I'd remove to be.

erak · 2018-03-23T11:49:36Z

Improved documentation. Should be ready to merge.

erak requested review from axic, chriseth and ekpyron March 5, 2018 15:44

chriseth reviewed Mar 5, 2018

View reviewed changes

ekpyron reviewed Mar 5, 2018

View reviewed changes

erak force-pushed the blockhash-global branch from 60c7413 to 92a8ad9 Compare March 5, 2018 18:32

axic reviewed Mar 5, 2018

View reviewed changes

erak force-pushed the blockhash-global branch from 0be0c6e to 43a75e4 Compare March 7, 2018 11:03

chriseth reviewed Mar 9, 2018

View reviewed changes

erak force-pushed the blockhash-global branch from 96769b4 to 4a449c1 Compare March 9, 2018 14:46

erak force-pushed the blockhash-global branch from 4a449c1 to ef85910 Compare March 12, 2018 10:30

chriseth reviewed Mar 12, 2018

View reviewed changes

chriseth suggested changes Mar 12, 2018

View reviewed changes

erak force-pushed the blockhash-global branch from ef85910 to 0b25f48 Compare March 12, 2018 13:52

chriseth approved these changes Mar 12, 2018

View reviewed changes

axic force-pushed the blockhash-global branch from 0b25f48 to c4dbed9 Compare March 15, 2018 01:19

axic suggested changes Mar 21, 2018

View reviewed changes

axic force-pushed the blockhash-global branch 2 times, most recently from e4b7644 to c5b6a2e Compare March 27, 2018 02:29

erak added 4 commits March 27, 2018 03:30

Moves blockhash function to global level.

ed63202

Adds unit tests for moved function.

be35a65

Updates docs for blockhash changes.

bddfa47

Changes deprecation and adjusts tests.

2c56e53

axic force-pushed the blockhash-global branch from c5b6a2e to 2c56e53 Compare March 27, 2018 02:30

axic approved these changes Mar 27, 2018

View reviewed changes

axic merged commit 32f0898 into develop Mar 27, 2018

axic deleted the blockhash-global branch March 27, 2018 10:40

Conversation

erak commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erak commented Mar 5, 2018

Uh oh!

chriseth commented Mar 5, 2018

Uh oh!

chriseth commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekpyron commented Mar 5, 2018

Uh oh!

erak commented Mar 7, 2018

Uh oh!

chriseth commented Mar 7, 2018

Uh oh!

erak commented Mar 7, 2018

Uh oh!

erak commented Mar 7, 2018

Uh oh!

chriseth commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erak commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

erak commented Mar 12, 2018

Uh oh!

chriseth commented Mar 12, 2018

Uh oh!

axic commented Mar 14, 2018

Uh oh!

chriseth commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth Mar 5, 2018 •

edited

Loading