[v0.11 feature] Built-in Reward Share #1581

msmania · 2023-09-07T00:56:37Z

Description

This patch implements the Built-in Reward Share feature which lets the network itself to distribute the relay/block rewards into multiple addresses. The change consists of the following parts.

Add RewardDelegators in Validator
Add RewardDelegators in MsgStake
Add a new command pocket nodes stakeNew
Distributes the fee of claim/proof transaction to a node's operator address
Add a new config prevent_negative_reward_claim not to claim a potential loss evidence.

This change is consensus-breaking. The new behavior is put behind a new feature key RewardDelegator. The new field is kept unavailable until activation.

The new structure of Validator or MsgStake is backward/forward compatible, meaning the new binary can still unmarshal data marshaled by an older binary, and vice versa. In other words, the network before RewardDelegator activation accepts an MsgStake transaction, ignoring the RewardDelegators field. And the new Validator structure can handle all historical states from genesis. Therefore this patch does not introduce a structure like 10.0Validaor as the NCUST patch did before.

Summary generated by Reviewpad on 26 Dec 23 01:21 UTC

This pull request includes changes to multiple files. Here is a summary of the diff:

The file common_test.go was modified to replace the import of math/rand with crypto/rand. Additionally, the comment // : deadcode unused was removed.
The file x/nodes/keeper/abci_test.go was modified to add and remove import statements, as well as comment out unnecessary code related to state conversion.
The file x/nodes/types/validator.go was modified to add an import, add a new field to the Validator struct, add a new function to create a validator from a message, modify several methods to include a new field in the output, and add a new struct and comment.
The file x/nodes/types/validator_test.go was modified to add import statements and a new test function.
The file msg_test.go was modified to add and remove import statements, add new test functions, and update existing test functions.
The file keeper_test.go was modified to add import statements, modify existing test functions, and add new test functions.
The file go.mod was modified to add and update package requirements.
The file handler.go was modified to add import statements and modify function implementations.
The file nodes.proto was modified to remove an import statement and add a new field to a message.
The file msg.go was modified to add import statements, add a new struct and function, and modify existing methods.
The file genesis_test.go was modified to add import statements and modify existing test functions.
The file rpc_test.go was modified to add and remove import statements, modify function implementations, and add test cases.
The file expectedKeepers.go was modified to remove comments and add a new method.
The file config.go was modified to add a new field to a struct.
The file msg.proto was modified to add a new field to a message.
The file LegacyValidator.go was modified to add a new method and update existing methods.
The file errors.go was modified to add new error codes and functions to handle them.
The file reward_test.go was modified to add import statements, add and update test functions.
The file util_test.go was modified to rearrange import statements and add new test functions.

Please review these changes and provide any necessary feedback. Let me know if you need more information or if there's anything else I can assist you with.

POKT-Discourse · 2023-09-08T17:17:14Z

This pull request has been mentioned on Pocket Network Forum. There might be relevant details there:

https://forum.pokt.network/t/unleashing-the-potential-of-pocket/4720/1

POKT-Discourse · 2023-10-13T22:31:51Z

This pull request has been mentioned on Pocket Network Forum. There might be relevant details there:

https://forum.pokt.network/t/unleashing-the-potential-of-non-custodial-node-running/4796/1

Olshansk · 2023-11-29T03:59:54Z

@msmania Please note that I updated the base to tokikuch/RTTM2 so its easier to review. We HAVE TO make sure to keep it up to date with that branch and MAKE SURE to update it back to main before meging.

Olshansk

Uploading a partial review (not complete yet)

app/cmd/cli/node.go

app/cmd/rpc/rpc_test.go

proto/x/nodes/msg.proto

proto/x/nodes/nodes.proto

x/nodes/handler.go

Olshansk

Still not a complete review but posting my additional comments so far.

codec/codec.go

x/nodes/keeper/params.go

x/nodes/keeper/reward_test.go

x/nodes/types/msg_test.go

x/nodes/keeper/valStateChanges.go

Olshansk

Reviewed everything except for x/nodes/keeper/reward_test.go and I think we're really close!

I don't know about you, but I think this reads SO much better than the previous iteration. Thanks for all the effort!

.circleci/config.yml

CONTRIBUTING.md

app/cmd/rpc/rpc_test.go

x/nodes/handler.go

x/nodes/keeper/reward.go

x/nodes/keeper/validator_test.go

x/nodes/keeper/reward.go

x/nodes/keeper/valStateChanges_test.go

The coming patch #1581 uses Go Generics that is available since Go 1.18. Since .github/workflows/Dockerfile pulls a container with Go 1.17, that patch breaks CI workflow. This patch upgrades Go version in all CI workflows to the latest version 1.21.

Tests to verify compatibility will come with a subsequent patch.

The existing `nodes stake` command is not very user-friendly for the following reasons: - Need to specify a subcommand custodial or non-custodial - Need to specify `isBefore8.0` parameter - Unclear about which wallet signs a message It's time to introduce a new stake command that is aligned with the latest node structure.

…tial loss

Olshansk

@msmania I took another look and I think this is as good of a review as I can do too (reaching PR exhaustion).

I added one small NIT, one #PUC request, but otherwise the only blocker is to get the CI green after we merge the other PR.

x/nodes/types/errors.go

The coming patch #1581 uses Go Generics that is available since Go 1.18. Since .github/workflows/Dockerfile pulls a container with Go 1.17, that patch breaks CI workflow. This patch upgrades Go version in all CI workflows to the latest version 1.21. ## Description  ### Summary generated by Reviewpad on 21 Dec 23 08:47 UTC This pull request upgrades the Go version in all CI workflows to version 1.21. The current version 1.18 is causing issues with the coming patch that uses Go Generics available since Go 1.18. The upgrade is done in the .circleci/config.yml file for the build, test, and trigger-pocket-core-deployments-branches sections. It is also done in the .github/workflows/Dockerfile file. Additionally, the go.mod and README.md files are updated to reflect the new Go version. The patch includes a total of 13 insertions and 10 deletions across 6 files.

msmania · 2023-12-22T00:39:30Z

@msmania I took another look and I think this is as good of a review as I can do too (reaching PR exhaustion).

I added one small NIT, one #PUC request, but otherwise the only blocker is to get the CI green after we merge the other PR.

Thank you! I created #1589 to get the CI green.

msmania · 2023-12-22T14:23:20Z

@msmania I took another look and I think this is as good of a review as I can do too (reaching PR exhaustion).

I added one small NIT, one #PUC request, but otherwise the only blocker is to get the CI green after we merge the other PR.

Comparing the test results of #1589 to the results of #1587, the following tests failed in both of the test runs.

TestValidateGenesis/Test_ValidateGenesis_8
TestDefaultParams

The difference is TestRPC_QueryUnconfirmedTxs, which I think is flaky and can be ignored as it failed on #1582 too as mentioned here. So I think this is good to be merged. What do you think, @Olshansk?

Olshansk

@msmania Going to approve this since we've iterated quite a bit and it's the last day before the holidays.

However, three requests:

Please make sure to get a 🟢 in RewardShare for CI stabilization #1589
Please use squash and merge and merging into main.
Please git merge instead of git rebase in future PRs. It makes it MUCH easier for the reviewer to leverage the "changes since last review" feature.

This patch fixes the following tests: - TestValidators_String - TestDefaultParams/Default_Test - TestValidateGenesis/Test_ValidateGenesis_8 This test expected a failure in validation because `GetParams` returned 50 for `MinSignedPerWindow`, but that bug was fixed. The expected result is a success now. and disabled `TestRPC_QueryUnconfirmedTxs` that was flaky. It passed on a local environment, but sometimes failed on GitHub CI.

msmania · 2023-12-23T22:56:02Z

#1589 got a green light.

msmania · 2023-12-24T01:01:38Z

@Olshansk Looks like a force-push revoked your approval. Can you sign off again?

Olshansk

@msmania Left one small nit / comment, so please tend to that before merging.

Note: No matter how many times I review this, I'm still a bit worried (i.e. chain halt) given the size and scope of the change, so let's make sure to do our due diligence in testing once we're in TestNet.

--- Regarding Git Flow ---

Bullet points with extra context below.

I left a comment related to this in one of the PRs a couple weeks ago with a bit more details [1]
For some extra "cred", even Mitchell Hashimoto understood the value of using this GitHub workflow
We documented this in the pocket repo but not the pocket-core one unfortunately. I'll make sure we transfer it into the poktroll repo
I want to emphasize that this is not a how Pocket does thing but a How GitHub expects users to use Git. A friend of mine is a PM there and I've brought this up a few times.

[1] #1581 (review)
[2] https://x.com/olshansky/status/1708314683738820998?s=20
[3] https://github.com/pokt-network/pocket/blob/main/docs/development/CODE_REVIEW_GUIDELINES.md

Olshansk · 2023-12-25T16:17:24Z

app/cmd/rpc/rpc_test.go

-	cleanup()
-	stopCli()
-}
+// Disabling the test because this is flaky.


Uncomment this and use t.skip("TODO: Figure out why this test is flaky") instead.

## Description This is a fix for a regression introduced by #1581. When a node stores a validator with the new `RewardDelegators` field, its marshaled product is not deterministic because the order of map iteration is not deterministic. As a result, the merkle tree of application.db can differ between nodes, leading to consensus failure. The proposed fix is to sort the keys of `RewardDelegators` by using the stable marshaler.  ### Summary generated by Reviewpad on 15 Jan 24 18:21 UTC This pull request includes changes to the `nodes.proto` file and a file diff. The changes in the `nodes.proto` file include: - Added an option `stable_marshaler` with a value of `true`. This option is used to make the order of `RewardDelegators` deterministic. - Updated the `Address` field to include additional options `(gogoproto.casttype)`, `(gogoproto.moretags)`, and `(gogoproto.jsontag)`. - Updated the `PublicKey` field to include additional options `(gogoproto.moretags)` and `(gogoproto.jsontag)`. - Updated the `jailed` field to include an additional option `(gogoproto.jsontag)`. The file diff includes the following changes: - Imported the "encoding/hex", "math/rand", "reflect", "strconv", and "time" packages. - Added a test case for marshalling RewardDelegators. - Added the "Test_Marshal_RewardDelegators" function. - Created variables and constants within the test function. - Generated random numbers to be used as reward delegator addresses. - Created and initialized a map, "delegatorMap", with random numbers as keys and their corresponding modulo 10 values as values. - Created a "Validator" struct with various fields and assigned values to them. - Marshaled the validator struct and ensured that the resulting hash is the same for every iteration of the test. Please let me know if there is anything specific you would like me to focus on.

POKT-Discourse · 2024-01-31T19:00:12Z

This pull request has been mentioned on Pocket Network Forum. There might be relevant details there:

https://forum.pokt.network/t/rc-0-11-1-upgrade-and-hi/5012/1

This patch implements a new feature Reward Delegators ([PIP-32](https://forum.pokt.network/t/pip32-unleashing-the-potential-of-non-custodial-node-running/4796); introduced by pokt-network/pocket-core#1581).

reviewpad bot added large Pull request is large waiting-for-review labels Sep 7, 2023

msmania self-assigned this Nov 14, 2023

msmania requested a review from Olshansk November 14, 2023 23:15

Olshansk changed the base branch from staging to tokikuch/RTTM2 November 29, 2023 03:58

Olshansk modified the milestones: Network Quality of Life, Network Cost Nov 29, 2023

msmania force-pushed the tokikuch/Built-in-Reward-Share branch from f6371ad to 67d95a2 Compare December 6, 2023 02:35

Olshansk requested changes Dec 14, 2023

View reviewed changes

Olshansk requested changes Dec 15, 2023

View reviewed changes

Base automatically changed from tokikuch/RTTM2 to staging December 15, 2023 08:23

msmania force-pushed the tokikuch/Built-in-Reward-Share branch 2 times, most recently from 2446c1c to 56e09be Compare December 20, 2023 12:57

msmania requested a review from Olshansk December 20, 2023 12:59

msmania force-pushed the tokikuch/Built-in-Reward-Share branch 2 times, most recently from 900363f to 36062dd Compare December 20, 2023 13:50

Olshansk requested changes Dec 21, 2023

View reviewed changes

msmania mentioned this pull request Dec 21, 2023

Upgrade Go version to 1.21 #1587

Merged

msmania force-pushed the tokikuch/Built-in-Reward-Share branch from 36062dd to 3031344 Compare December 21, 2023 15:34

kinesis-agent added 6 commits December 21, 2023 07:34

Part 1 - Remove an unused function ConvertValidatorsState

6ffc285

Part 2 - Introduce a new field Delegators to Validator

daaeeae

Part 3 - Introduce a new field Delegators to MsgStake

4946c47

Tests to verify compatibility will come with a subsequent patch.

Part 5 - Introduce an option not to claim an evidence that is a poten…

e061fee

…tial loss

Part 6 - Delegator distribution for relay rewards

cab0a0e

msmania force-pushed the tokikuch/Built-in-Reward-Share branch from 3031344 to 15e2488 Compare December 21, 2023 15:34

msmania requested a review from Olshansk December 21, 2023 15:37

Olshansk requested changes Dec 21, 2023

View reviewed changes

x/nodes/types/errors.go Outdated Show resolved Hide resolved

msmania force-pushed the tokikuch/Built-in-Reward-Share branch from 15e2488 to c7b8fd8 Compare December 22, 2023 00:20

msmania requested a review from Olshansk December 22, 2023 00:38

Olshansk previously approved these changes Dec 22, 2023

View reviewed changes

Olshansk mentioned this pull request Dec 22, 2023

RewardShare for CI stabilization #1589

Closed

msmania added 2 commits December 23, 2023 14:49

address comments

d91add1

msmania dismissed Olshansk’s stale review via bc86da9 December 23, 2023 22:49

msmania force-pushed the tokikuch/Built-in-Reward-Share branch from 8591f71 to bc86da9 Compare December 23, 2023 22:49

msmania requested a review from Olshansk December 24, 2023 08:44

Olshansk previously approved these changes Dec 25, 2023

View reviewed changes

Skip TestRPC_QueryUnconfirmedTxs instead of commenting

444b54d

msmania dismissed Olshansk’s stale review via 444b54d December 26, 2023 01:20

msmania requested a review from Olshansk December 26, 2023 01:37

Olshansk approved these changes Dec 26, 2023

View reviewed changes

msmania merged commit 58217f2 into staging Dec 26, 2023
1 check passed

msmania deleted the tokikuch/Built-in-Reward-Share branch December 26, 2023 05:18

msmania mentioned this pull request Jan 14, 2024

Sort RewardDelegators when marshaling a validator #1591

Merged

msmania mentioned this pull request Mar 11, 2024

TxBuilder: TransactionBuilder.nodeStake takes rewardDelegators pokt-network/pocket-js#102

Merged

Olshansk mentioned this pull request Apr 25, 2024

[Utility][Morse Parity] Built In Revenue Share pokt-network/poktroll#496

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v0.11 feature] Built-in Reward Share #1581

[v0.11 feature] Built-in Reward Share #1581

msmania commented Sep 7, 2023 •

edited by reviewpad bot

Loading

POKT-Discourse commented Sep 8, 2023

POKT-Discourse commented Oct 13, 2023

Olshansk commented Nov 29, 2023

Olshansk left a comment

Olshansk left a comment

Olshansk left a comment

Olshansk left a comment

msmania commented Dec 22, 2023

msmania commented Dec 22, 2023

Olshansk left a comment

msmania commented Dec 23, 2023

msmania commented Dec 24, 2023

Olshansk left a comment •

edited

Loading

Olshansk Dec 25, 2023

POKT-Discourse commented Jan 31, 2024

[v0.11 feature] Built-in Reward Share #1581

[v0.11 feature] Built-in Reward Share #1581

Conversation

msmania commented Sep 7, 2023 • edited by reviewpad bot Loading

Description

Summary generated by Reviewpad on 26 Dec 23 01:21 UTC

POKT-Discourse commented Sep 8, 2023

POKT-Discourse commented Oct 13, 2023

Olshansk commented Nov 29, 2023

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

msmania commented Dec 22, 2023

msmania commented Dec 22, 2023

Olshansk left a comment

Choose a reason for hiding this comment

msmania commented Dec 23, 2023

msmania commented Dec 24, 2023

Olshansk left a comment • edited Loading

Choose a reason for hiding this comment

Olshansk Dec 25, 2023

Choose a reason for hiding this comment

POKT-Discourse commented Jan 31, 2024

msmania commented Sep 7, 2023 •

edited by reviewpad bot

Loading

Olshansk left a comment •

edited

Loading