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

Contract call optimization - remove round trip #944

Merged
merged 5 commits into from Aug 30, 2018
Merged

Contract call optimization - remove round trip #944

merged 5 commits into from Aug 30, 2018

Conversation

medvedev1088
Copy link
Contributor

@medvedev1088 medvedev1088 commented Jul 3, 2018

What was wrong?

When calling a contract function on latest block, the lib unnecessarily calls getBlock('latest')

How was it fixed?

Remove a round trip when block number is latest, earliest or pending when calling a contract function.

@pipermerriam
Copy link
Member

Test failure appears unrelated (due to import sorting).

@voith
Copy link
Contributor

voith commented Jul 3, 2018

@pipermerriam Test failures have been resolved by @carver in #893. However, it is waiting to be merged.

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

Cool, thanks for the contribution! Are you up for adding a test for a block identifier where the number is -1 - web3.eth.getBlock('latest').number ?

web3/contract.py Outdated
if block_identifier >= 0:
return block_identifier
else:
return last_block + block_identifier + 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the chain is 3 blocks long (so the latest number is 2), then -3 should be a valid identifier. One way to fix that edge case, and hopefully in a clean way to read, is:

last_block = web3.eth.getBlock('latest').number

if block_identifier >= 0:
  block_num = block_identifier
else:
  block_num = last_block + block_identifier + 1

if 0 <= block_num <= last_block:
  return block_num
else:
  raise BlockNumberOutofRange

This is just long enough that it could go into its own method, like parse_block_identifier_int() and keep parse_block_identifier as a super simple dispatch method.

@medvedev1088
Copy link
Contributor Author

@carver I fixed the edge case as you suggested, but unfortunately I will not have time to write tests for it.

BTW, what do you think about submitting a feature request to geth/parity to support negative block identifiers? In this case there will be no additional round trips even when block identifier is integer.

@carver
Copy link
Collaborator

carver commented Jul 10, 2018

I fixed the edge case as you suggested, but unfortunately I will not have time to write tests for it.

Thanks, as soon as someone gets time to add the test, we'll get this merged.

BTW, what do you think about submitting a feature request to geth/parity to support negative block identifiers? In this case there will be no additional round trips even when block identifier is integer.

Yeah, definitely worth a discussion. 👍 Feel free to @ mention me if you decide to open up issues and I'll +1 it

@dylanjw
Copy link
Contributor

dylanjw commented Jul 24, 2018

Didnt mean to add the breaking change label. Hand must have slipped.


def test_parse_block_identifier_int(web3):
with pytest.raises(BlockNumberOutofRange):
parse_block_identifier_int(web3, -2)
Copy link
Contributor

@dylanjw dylanjw Aug 6, 2018

Choose a reason for hiding this comment

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

[...] adding a test for a block identifier where the number is -1 - web3.eth.getBlock('latest').number ?

@carver Is this what you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, as I read it, -1 - w3.eth.getBlock('latest').number should be block zero.

Say there are currently 3 blocks:

Block Number Negative Index Special
0 -3
1 -2
2 -1 latest

I think the test could be:

last_num = w3.eth.getBlock('latest').number
assert w3.eth.getBlock(0) == w3.eth.getBlock(-1 - last_num)

web3/contract.py Outdated
@@ -1411,6 +1407,20 @@ def parse_block_identifier(web3, block_identifier):
raise BlockNumberOutofRange


def parse_block_identifier_int(web3, block_identifier_int):
last_block = web3.eth.getBlock('latest').number
Copy link
Collaborator

@carver carver Aug 7, 2018

Choose a reason for hiding this comment

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

Arguably, it's also a little heavy-weight to force this latest block call when you use a positive block number, just to do local validation. The node will already fail pretty straightforwardly, if you supply too large a block number.

@dylanjw
Copy link
Contributor

dylanjw commented Aug 16, 2018

@carver ok to merge? EDIT: doh!

@carver
Copy link
Collaborator

carver commented Aug 16, 2018

(except for the tests, of course ;))

def test_parse_block_identifier_int(web3):
last_num = web3.eth.getBlock('latest').number
assert web3.eth.getBlock(0) == web3.eth.getBlock(-1 - last_num)
assert web3.eth.getBlock(0) == web3.eth.getBlock(
parse_block_identifier_int(web3, -1 - last_num))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this change necessary? Isn't getBlock supposed to accept the raw int?

Copy link
Contributor

@dylanjw dylanjw Aug 26, 2018

Choose a reason for hiding this comment

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

web3.eth.getBlock doesnt accept negative block identifiers. parse_block_identifier_int will translate negative identifiers into the block number. Maybe the assertion should just be:

assert 0 == parse_block_identifier_int(web3, -1 - last_num)

Copy link
Member

Choose a reason for hiding this comment

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

It's very strange that parse_block_identifier would normalize a negative number to zero. That strikes me as unexpected behavior and likely to result in accidental errors. It seems like if the number if negative then it should raise a ValidationError or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse_block_identifier has logic to handle negative block numbers in a similar way Python does it for lists, e.g. parse_block_identifier(-2) will return the block before the last one.
Currently this method would call getBlock('latest') for every invocation of a contract. What this pull request does is eliminates this extra call when block identifier (for the contract function call) is not numeric, e.g. 'latest'.
In my opinion, handling negative block identifiers for contract function calls should be removed for 3 reasons: (1) as @pipermerriam said it's unexpected behavior, (2) it's inconsistent with web3.getBlock() which doesn't handle negative block numbers, (3) it affects performance. The problem with removing this logic is it will break backward compatibility.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it seems to have been added in 9f23db0

I don't see any rationale in the related PR or issue for why a negative index was particularly helpful or important. I'm happy to see it get dropped in v5. I also understand why this test did what it did now. So 👍 from me to merge this PR (which isn't making the situation worse) as is.

Although it's probably worth adding a comment on the test, mentioning that it is converting the negative index from the latest block back to zero. Also saying that the test has to do the conversion explicitly because transaction calls allow this negative index, but getBlock() doesn't.

@dylanjw dylanjw merged commit d1495f3 into ethereum:master Aug 30, 2018
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.

None yet

5 participants