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

eth: hints for staking actions #1845

Merged
merged 1 commit into from
May 5, 2021
Merged

eth: hints for staking actions #1845

merged 1 commit into from
May 5, 2021

Conversation

kyriediculous
Copy link
Contributor

@kyriediculous kyriediculous commented Apr 16, 2021

What does this pull request do? Explain your changes. (required)
This PR adds support for hints when performing staking actions on the Livepeer smart contracts.

Specific updates (required)

  • Add wrappers function for staking action to eth/Client.go

How did you test each of these updates (required)

  • test on rinkeby and confirm gas used is lower and hints are correct

Does this pull request close any open issues?
Fixes #1567

Checklist:

@kyriediculous
Copy link
Contributor Author

Wanted to rework the helpers to get the hints first so we could avoid repeating getting the transcoder pool, transcoder pool size, ... .

Then I realised the reason why we went with the functional programming approach for those is because of testability.

Using a method on client would prevent the method to be testable with mock data with the current state of that implementation.

@kyriediculous kyriediculous marked this pull request as ready for review April 16, 2021 20:08
@kyriediculous kyriediculous requested a review from yondonfu April 16, 2021 20:08
@kyriediculous
Copy link
Contributor Author

Added a fix in the way we calculate the stake amounts, previously posPrev always ended up being 0x00...

tested on rinkeby and works now

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

tested on rinkeby and works now

Can you confirm that the calculated hints are working as expected and the gas costs are reduced relative to if the transactions were submitted without hints (i.e. by posting gas cost differences)?

eth/client.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
@kyriediculous
Copy link
Contributor Author

State before

image

Self bond 5 LPT (16.66 + 5 = 21.66 LPT) :

Invoking transaction: "bondWithHint". Inputs: "_amount: 5000000000000000000  _to: 0x3566eFB780B6e9534399d9F8BeA1ac0F2B179d83  _oldDelegateNewPosPrev: 0x0000000000000000000000000000000000000000  _oldDelegateNewPosNext: 0x0000000000000000000000000000000000000000  _currDelegateNewPosPrev: 0xDAC817294c0c87ca4fA1895eF4b972EAde99f2fd  _currDelegateNewPosNext: 0xE358c19D115f0BCCdB51B5b7a04be24808E89418"  Hash: "0xd403ade95d4b70b1fc9d6dc84a438775ae8529ebe8acfd1e01f733319046abcb".

Transcoder list after transaction (hints match the new position)

image

BondWithHint Transaction: https://rinkeby.etherscan.io/tx/0xd403ade95d4b70b1fc9d6dc84a438775ae8529ebe8acfd1e01f733319046abcb

Bond Transaction https://rinkeby.etherscan.io/tx/0x6b3c51ee116b0209b577c6fb13723f2b3c109265e3554469551e608db4c13ea3

For some reason the regular Bond transaction is ~20K gas cheaper, despite the hints being correct. The state update is the same.

Unbond 5 LPT

Invoking transaction: "unbondWithHint". Inputs: "_amount: 5000000000000000000  _newPosPrev: 0x0b041CcE35E3C08c5E2AFb76A25b6a53421abBc1  _newPosNext: 0xA6ddcF04D8EbF15E66440eB41C2C2075Ff358465"  Hash: "0x0c22c8f3d3936ad9273e6ee2408ff55d723d5652e2629c5db6377db0f8599ca0".

Hints should match the prev and nex positions of the original state (first picture) , they do.

UnbondWithHint transaction https://rinkeby.etherscan.io/tx/0x0c22c8f3d3936ad9273e6ee2408ff55d723d5652e2629c5db6377db0f8599ca0

Unbond transaction
0xb6de7b28f9a25361eea490ec61f4515b1a38705e9368554dd72f2a7724e63def

UnbondWithHint is about 110K gas cheaper

RebondWithHint

Invoking transaction: "rebondWithHint". Inputs: "_unbondingLockId: 1  _newPosPrev: 0xDAC817294c0c87ca4fA1895eF4b972EAde99f2fd  _newPosNext: 0xE358c19D115f0BCCdB51B5b7a04be24808E89418"  Hash: "0x462a94b8bec172deafd584dc3886a5c2a36a011b525e14f86a8543a14e84a364".

Hints should again match the ones from the BondWithHint transaction, they do

RebondWithHint transaction https://rinkeby.etherscan.io/tx/0x462a94b8bec172deafd584dc3886a5c2a36a011b525e14f86a8543a14e84a364

Rebond transaction https://rinkeby.etherscan.io/tx/0xad50716c5883dced9606ea6debed524d089d97c2896c37cab2f32f3d3b20f51f

RebondWithHint is about ~27k gas cheaper

@yondonfu
Copy link
Member

For some reason the regular Bond transaction is ~20K gas cheaper, despite the hints being correct. The state update is the same.

Can you investigate why this is the case and confirm if the bondWithHint() tx is more expensive than the bond() tx due to a problem with the hints or some other reason?

From glancing at the Rinkeby txs, it looks like the two txs for bonding were submitted from the same account one after the other around the same time. Could it be the case that the first bondWithHint() tx changed the transcoder pool ordering such that the second bond() tx (without hints) required less iterations in the transcoder pool (or even no iterations at all) and as a result the bondWithHint() tx ended up being more expensive due to the gas costs associated with including/reading the hints on-chain? I bring this up because we should make sure (if we're not doing so already) that when comparing txs with and without hints that the on-chain transcoder pool state is the same before each tx to make it a fair comparison.

@kyriediculous
Copy link
Contributor Author

kyriediculous commented Apr 21, 2021

The BondWithHint transaction includes the call for autoClaimEarnings

The regular Bond transaction took place in the same round thus skipped UpdateDelegatorWithEarnings

I ran these transactions through the tenderly execution tracer and this confirms as much. As you can see in the second image there is no call to UpdateDelegatorWithEarnings and pendingStakeAndFees. So In reality the BonWithHint function would have been several thousands of gas cheaper if they would have been identical.

  • BondWithHint

image

  • Bond

image

eth/client.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/client.go Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
@kyriediculous kyriediculous requested a review from yondonfu April 29, 2021 12:21
eth/client.go Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM after squashing

@kyriediculous kyriediculous merged commit d3dcd11 into master May 5, 2021
@kyriediculous kyriediculous deleted the nv/hints branch May 5, 2021 12:36
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.

staking actions with hints in the CLI
2 participants