-
Notifications
You must be signed in to change notification settings - Fork 103
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
[v0.11 feature] Per-chain Relays To Token Multiplier #1580
Conversation
This patch imports pokt-network/tendermint@5931015.
The parameter `pos/MinSignedPerWindow` defines a proportion, but the function `MinSignedPerWindow` returns the number of blocks, calculated as `pos/MinSignedPerWindow * pos/SignedBlocksWindow`. This is confusing. To resolve this discrepancy, we had "custom logic" to convert the return value back to the parameter value. This is unnecessary calculation. There was indeed a bug in `PosKeeper.GetParams` that returned the return value from `k.MinSignedPerWindow(ctx)` without the "custom logic". As a result, the value of `min_signed_per_window` in the response from /v1/query/nodeparams was always wrong. To remove this confusing design, this patch does: - Rename the function `MinSignedPerWindow` to `MinBlocksSignedPerWindow` - Add a new `MinSignedPerWindow` function returning the exact parameter value in decimal as the other parameter getter functions do
The function `SetParamSet` writes parameters to application db, where we have a special logic to prevent parameters introduced after genesis from being written in blocks before those parameters are introduced. However, using `IsOnUpgradeHeight` in this logic is wrong because all post-genesis parameters we have are activated by a feature name, not when the protocol version is upgraded. As a result, we had never exercised this logic. This didn't cause any real problem yet because we never call `SetParamSet` for post-genesis parameters before they're introduced. We should do the right thing before this bug causes problems in the future.
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 |
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-as-universal-rpc-provider/4797/1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall: LGTM! Great change. Simple & powerful!
- Left some small NITs, comments and questions but nothing important. Just want to go over it before merging
- Make sure to update the documentation (when we're closer to launch) on how to execute the tx commands with the marshalled json map
- The ONLY concern I have is related to the cleanup you did related to the
MinSignedPerWindow
. I don't know why we had this before and wanted to understand your confidence level on it?
// custom logic for minSignedPerWindow
params.MinSignedPerWindow = params.MinSignedPerWindow.QuoInt64(params.SignedBlocksWindow)
@@ -25,7 +26,7 @@ func BeginBlocker(ctx sdk.Ctx, req abci.RequestBeginBlock, k Keeper) { | |||
// store whether or not they have actually signed it and slash/unstake any | |||
// which have missed too many blocks in a row (downtime slashing) | |||
signedBlocksWindow := k.SignedBlocksWindow(ctx) | |||
minSignedPerWindow := k.MinSignedPerWindow(ctx) | |||
minSignedPerWindow := k.MinBlocksSignedPerWindow(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In x/nodes/keeper/params.go
, we use MinSignedPerWindow
and here we use MinBlocksSignedPerWindo
.
Just confirming that this is intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. As described in the Part2 commit's message, the function k.MinSignedPerWindow
is renamed to k.MinBlocksSignedPerWindow
. This function returns int64
, the number of blocks.
The parameter value MinSignedPerWindow
(not a function), which is BigDec
and the current value is 0.6
, is used in only one place, GetParams
in x/nodes/keeper/params.go.
@@ -107,8 +107,6 @@ func (k Keeper) ConvertState(ctx sdk.Ctx) { | |||
panic(err) | |||
} | |||
k.Cdc.SetUpgradeOverride(true) | |||
// custom logic for minSignedPerWindow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with this whole manual update for MinSignedPerWindow so it's the only piece that's giving me a bit of "unease" but I don't know how to confirm/validate it.
Is your confidence level high on these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ONLY concern I have is related to the cleanup you did related to the MinSignedPerWindow. I don't know why we had this before and wanted to understand your confidence level on it?
For the details, please check the commit message here.
The reason why we have this "custom logic" is the param SignedBlocksWindow
is a proportion (= 0.6 in the mainnet), while the function SignedBlocksWindow
is expected to return the number of blocks in a window (= 6 in the mainnet). This custom logic converts the proportion into the number of blocks by multiplying SignedBlocksWindow
. This patch reconciles this discrepancy under the name SignedBlocksWindow
.
The behavior is clear and the regression risk is low.
@msmania Ty for linking to the commit messages. I don't want this to become a "developer workflow" discussion since I know everyone has opinions/preferences on how to use git, github, IDEs, etc. I'm sharing mine below not in expectation of others adopting it, but just so you have context/visibility into how I work and I will accommodate your workflow as well.
|
Got it. I personally thought one gigantic commit would be hard to be reviewed, or to be analyzed after merge in the future, so I split a patch to several PartX commits, following a convention in the projects I used to work. Anyway, thank you for signing off! Let's merge this! |
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 is a fix for a regression introduced by Per-chain RTTM (#1580). When we store a map-type parameter, we use `Codec.MarshalJSON` that doen't guarantee the order of fields in a map. This causes consensus failure because a different order of fields in the parameter store generates a different merkle tree (= AppHash). The proposed fix is to canonicalize the marshaled bytes by calling `MustSortJSON` before storing it in the tree.
This is a fix for a regression introduced by Per-chain RTTM (#1580). When we store a map-type parameter, we use `Codec.MarshalJSON` that doen't guarantee the order of fields in a map. This causes consensus failure because a different order of fields in the parameter store generates a different merkle tree (= AppHash). The proposed fix is to canonicalize the marshaled bytes by calling `MustSortJSON` before storing it in the tree.
This is a fix for a regression introduced by Per-chain RTTM (#1580). When we store a map-type parameter, we use `Codec.MarshalJSON` that doen't guarantee the order of fields in a map. This causes consensus failure because a different order of fields in the parameter store generates a different merkle tree (= AppHash), resulting in chainhalt. The proposed fix is to canonicalize the marshaled bytes by calling `MustSortJSON` before storing it in the tree. Changing `Subspace.Set` is enough and a change in ` Subspace.SetWithSubkey` won't be necessary because it's not used from anywhere, but we should keep consistency. reviewpad:summary
Description
This patch introduces a new network parameter
pos/RelaysToTokensMultiplierMap
so that we can set a custom RTTM per chain. The existing parameterpos/RelaysToTokensMultiplier
serves the default RTTM if a custom RTTM is not set.This change is consensus-breaking. The new behavior is put behind a new feature key
RTTM2
. The new parameter is kept unavailable until activation.The patch also contains two tests:
Test_RTTM2_ChangeParamValue
to make sure the new parameter is kept unavailable until activating theRTTM2
feature key.TestKeeper_RewardForRelaysPerChain
to make sure the amount of relay rewards are calculated with a custom RTTM in the new parameter if it's set.Summary generated by Reviewpad on 07 Sep 23 00:52 UTC
This pull request contains the following changes:
The addition of a new test function called
TestKeeper_RewardForRelaysPerChain
. This test function verifies the rewards generated for different scenarios related to relays per chain.Modifications to the file "expectedKeepers.go" were made. A new function called "RewardForRelaysPerChain" was added, which calculates and returns a value of type
sdk.BigInt
.Changes were made to the file "module.go" related to activating additional parameters and setting a minimum signed per window value.
The file "keeper_test.go" includes the addition of a new test function called "Test_RTTM2_ChangeParamValue", which tests the change of a parameter value after a certain activation height.
A new file "proof.go" was modified to enhance the functionality and reliability of the
ExecuteProof
function in handling relay proofs.The file "param.go" now includes additional tracking of parameters introduced after genesis and skips writing them to the database until the corresponding feature activation height is reached.
The file "nodes.go" includes changes to the
AwardCoinsForRelays
function, enabling it to award coins for relays completed per chain.Modifications were made to the
PosKeeper
interface in the file "expectedKeepers.go", adding a new function calledRewardForRelaysPerChain
.The file "service_test.go" includes changes to the
RewardForRelays
method in theMockPosKeeper
struct, allowing coins to be awarded for relays completed per chain.The file "abci.go" in the
x/nodes/keeper
package includes import statement additions and changes to variable names.The go.sum file shows updates to dependencies related to the "github.com/pokt-network/tendermint" package.
These changes enhance functionality, introduce new features, and improve reliability in various parts of the codebase.