Resolve harmony-one/bounties#77: Staking precompiles #3906
Conversation
Create write capable precompiles that can perform staking transactions Add hard fork logic (EpochTBD) for these precompiles Tests for new code with at least 80% unit test coverage Staking library + tests in MaxMustermann2/harmony-staking-precompiles
From Solidity, use abi.encodeWithSelector and match it against the exact ABI of the functions. This allows us to remove the need for a directive (32) being encoded, and thus saves 28 bytes of data.
As discussed with Jacky on harmony-one/harmony#3906
As discussed with Jacky on harmony-one#3906
…rmony-one-main
Merge upstream from Harmony
Please see comment in core/vm/contracts_write.go RequiredGas
Add the StakeMsgs to ProcessorResult and cascade them in insertChain
Since smart contracts can no longer beecome validators, this field is superfluous. Remove it from the Wrapper structure, and do not assign it a value when creating a validator. Build and goimports checked
|
Looks very good. Could you please add a test case of multiple delegations/undelegations/claimReward in a single transaction to see whether the result is expected? |
There was a problem hiding this comment.
This PR is not safe to merge because the revert mechanism is broken for statedb.UpdateValidatorWrapper. We need to implement that first so that this PR change can be merged. I am truly sorry for the inconvenience. @rlan35 Any thought on reverting UpdateValidatorWrapper? Another bounty maybe?
| @@ -45,12 +45,20 @@ type ( | |||
| // GetVRFFunc returns the nth block vrf in the blockchain | |||
| // and is used by the precompile VRF contract. | |||
| GetVRFFunc func(uint64) common.Hash | |||
| // Below functions are used by staking precompile / state transition | |||
| CreateValidatorFunc func(db StateDB, stakeMsg *stakingTypes.CreateValidator) error | |||
There was a problem hiding this comment.
We may also remove CreateValidator and EditValidator
There was a problem hiding this comment.
why removing create and edit validator ? that would be useful for the node runner to manage their validator via metamask
There was a problem hiding this comment.
We may also remove CreateValidator and EditValidator
These cannot be removed because they are used in state_transition.go as err = st.evm.CreateValidator(st.evm.StateDB, stkMsg) and err = st.evm.EditValidator(st.evm.StateDB, stkMsg); and the types need to be defined for use in the evm object.
(1) Comments to start with function names (2) Comments for public variables (3) Comment to match function name RunPrecompiledContract (4) Clarify that CreateValidatorFunc + EditValidatorFunc are still used
This has been added, please look at
Fair point, I would be happy to work on the bounty if / when it is made. |
|
The bounty of revert mechanism for |
|
Nothing of major value to add other than I am very eagerly looking forward to seeing this implemented! |
|
As discussed with @rlan35 over Discord, I have added a staking migration precompile as well as one to fetch the current epoch. The logs for the integration tests are updated in the top comment, and the test coverage for files modified by this PR has been updated here. Regarding the failed build, I believe the reason is that the one on |
| blockNum := block.Number() | ||
| for _, stakeMsg := range stakeMsgs { |
There was a problem hiding this comment.
StakeMsgs are from Normal Transactions which is processed before Staking transaction. So this change is still consistent with the ordering of the processing. Right? No non-deterministic result here.
There was a problem hiding this comment.
Yes, but you are right that it needs to be thoroughly tested. I would appreciate it if this change spends a bit of time on testnet so I (and others) can verify the behaviour.
There was a problem hiding this comment.
Looking forward to testing on testnet. Nice work so far.
Merge the two precompiles into one, add gas calculation for migration precompile. Move epoch precompile to 251 as a result. When migrating, add undelegations to `To`'s existing undelegations, if any match the epoch.
In response to review comments, add tests for migration gas wherein there are 0/1/2 delegations to migrate. Add the index out of bound check to migration gas calculator and remove panics. Lastly, re-sort migrated undelegations if no existing undelegation in the same epoch was found on `To`.
|
is there any estimate for the epoch that enables this? |
|
The amount (e.g. for CollectReward) in the log emitted is using a non-fixed length integer. This will make it difficult for the client (e.g. web3 libraries) to parse the log (and capture the value) because it would be expecting fixed length integers (e.g. uint256, uint64, etc.) Line 301 in 027896a |
|
Also the event's topic hash is not following the standard way: It is computed from the hash of the name only. The standard way is the full signature, including parameter types (e.g. keccak256( |
|
This was not changed by me; I moved the code from harmony/core/state_transition.go Lines 544 to 549 in 5abe070 @rlan35 RJ, can you comment if this is something that can be changed ? For consistency with prior events, it should be changed with a fork. We can replace @polymorpher Can you please tell me what this is needed for, and if the proposed read only staking precompile can help instead? |
|
It's for client side apps to check how much reward was collected in a particular transaction. See polymorpher/one-wallet#251 (comment) for a visual output. My current workaround is to manually pad the data when event name equals a pre-determined one. polymorpher/one-wallet@a2f9245 Also I am computing the expected topic hash of precompiled events in a special manner |
|
Thanks, this will be updated when the read-only staking precompile is merged. I will give you a heads up. |
This PR adds staking precompiled contracts to the Harmony EVM, at address 0xFC after a hard fork at
EpochTBD. The precompiles can be called from Solidity by usingabi.encodeWithSelectoron the required parameters to produce the arguments for assemblycall. This will allow any contract to participate in staking. 3/5 directives (Delegate,UndelegateandCollectRewardsare accessible from the precompiled contract, which follows exactly the same logic as the originalApplyStakingMessage.A sample Solidity implementation is outlined here. The repository also contains integration tests for the (modified) node; the test report is available below. Note that a contract may only use the precompile for its own address; otherwise, the node will return an error indicating address mismatch. One instance of such a test case is covered in the sample implementation as
test_delegate_fail_malicious. Additionally, I have added a test case to ensure that the precompile can be called multiple times in one transaction (test_multiple_calls_success). This can be used, for example, by a staking contract, to delegate 50% of its ONE to one validator, 25% to another, and 25% to a third validator - all in a single transaction hash.Commit message:
Create write capable precompiles that can perform staking transactions
Add hard fork logic (EpochTBD) for these precompiles
Tests for new code with at least 80% unit test coverage
Staking library + tests in MaxMustermann2/harmony-staking-precompiles
Issue
harmony-one/bounties#77 | Gitcoin
Bounty Requirements
1.1 Core protocol
Create ValidatorEdit Validator1.2 Smart contract
1.3 Test reports
Test
Unit Test Coverage
Package-wide coverage containing only the files affected by this PR is available here. Save and open in your browser.
Before: 3657a1d
After: 92cd2fd
Test/Run Logs
For the Go node, refer to this link which contains the test coverage for only the files modified by this PR; please save and open this in your browser.
For the smart contract staking library, I copy the logs below. These logs cover all the edge cases of staking, as requested in the bounty. The instructions to run these tests are in the repository README, and the edge cases are described here. These logs are as of MaxMustermann2/harmony-staking-precompiles@5d87fe9 and 92cd2fd.
Operational Checklist
Does this PR introduce backward-incompatible changes to the on-disk data structure and/or the over-the-wire protocol?. (If no, skip to question 8.)
No.
Describe the migration plan.. For each flag epoch, describe what changes take place at the flag epoch, the anticipated interactions between upgraded/non-upgraded nodes, and any special operational considerations for the migration.
Describe how the plan was tested.
How much minimum baking period after the last flag epoch should we allow on Pangaea before promotion onto mainnet?
What are the planned flag epoch numbers and their ETAs on Pangaea?
What are the planned flag epoch numbers and their ETAs on mainnet?
Note that this must be enough to cover baking period on Pangaea.
What should node operators know about this planned change?
Does this PR introduce backward-incompatible changes NOT related to on-disk data structure and/or over-the-wire protocol? (If no, continue to question 11.)
No.
Does the existing
node.shcontinue to work with this change?What should node operators know about this change?
Smart contracts can now use the staking functions of the chain.
Does this PR introduce significant changes to the operational requirements of the node software, such as >20% increase in CPU, memory, and/or disk usage?
No.