perf!: Make slashing not write sign info every block #19458
Conversation
|
Warning Rate Limit Exceeded@ValarDragon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 18 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe significant change involves deprecating the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (2)
- proto/cosmos/slashing/v1beta1/slashing.proto (1 hunks)
- x/slashing/keeper/infractions.go (5 hunks)
Additional comments: 3
proto/cosmos/slashing/v1beta1/slashing.proto (1)
- 21-22: The change to deprecate the
index_offsetfield and mark it as reserved is correctly implemented following protobuf best practices. This prevents field number reuse and potential backward compatibility issues, aligning with the PR's objective to optimize the slashing mechanism.x/slashing/keeper/infractions.go (2)
- 61-68: The logic to compute the index for missed blocks using
(height - signInfo.StartHeight) % signedBlocksWindowis a clear and efficient way to map the validator's start height to index 0 and ensure the index stays within the range of[0, SignedBlocksWindow). This change supports the PR's goal of optimizing the slashing mechanism by making the computation of missed blocks more efficient.- 84-90: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [76-97]
Introducing the
editedSignInfoflag to track changes to the signing info and ensure it is only updated when necessary is a smart optimization. This approach reduces unnecessary writes to the database, aligning with the PR's objective to improve performance in the slashing mechanism. However, ensure that all scenarios wheresignInfomight need to be updated are covered by the conditions that seteditedSignInfoto true.
x/slashing/keeper/infractions.go
Outdated
| } | ||
|
|
||
| // Set the updated signing info | ||
| return k.ValidatorSigningInfo.Set(ctx, consAddr, signInfo) | ||
| if editedSignInfo { | ||
| return k.ValidatorSigningInfo.Set(ctx, consAddr, signInfo) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [134-209]
The logic for punishing validators who have missed too many blocks, including slashing, jailing, and resetting the missed blocks counter and bitmap, is correctly implemented. However, the removal of signInfo.IndexOffset update seems to be an oversight since IndexOffset has been deprecated and should no longer be used or updated. This aligns with the PR's objective to deprecate unnecessary fields and optimize the slashing process. Ensure that any remaining references to IndexOffset are removed or replaced as needed.
- signInfo.IndexOffset = 0
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (7)
- api/cosmos/accounts/interfaces/account_abstraction/v1/interface.pulsar.go (2 hunks)
- api/cosmos/slashing/v1beta1/slashing.pulsar.go (15 hunks)
- proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto (1 hunks)
- proto/cosmos/slashing/v1beta1/slashing.proto (1 hunks)
- x/accounts/interfaces/account_abstraction/v1/interface.pb.go (2 hunks)
- x/slashing/CHANGELOG.md (1 hunks)
- x/slashing/types/slashing.pb.go (7 hunks)
Files not summarized due to errors (1)
- api/cosmos/slashing/v1beta1/slashing.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (2)
- proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto
- x/accounts/interfaces/account_abstraction/v1/interface.pb.go
Files skipped from review as they are similar to previous changes (1)
- proto/cosmos/slashing/v1beta1/slashing.proto
Additional comments: 7
x/slashing/CHANGELOG.md (3)
- 32-32: The improvements listed under the "Improvements" section are clear and well-documented. Each entry correctly references the PR number, providing easy access to further details. However, it's important to ensure that the descriptions are concise and accurately reflect the changes made. For instance, the entry for PR perf!: Make slashing not write sign info every block #19458 could be slightly rephrased for clarity:
- Avoid writing SignInfo's for validator's who did not miss a block. (Every BeginBlock) + Avoid writing SignInfo for validators who did not miss a block in every BeginBlock.This minor adjustment improves readability and grammatical correctness.
- 32-32: The entry for PR refactor: remove panic usage in keeper methods #18636 introduces an important error handling improvement. It's crucial to ensure that the changelog accurately communicates the nature of the change without implying that the previous behavior was incorrect. The current wording is appropriate as it highlights the enhancement in error handling rather than framing it as a correction of a bug. This distinction is important for users understanding the nature of the change.
- 32-32: For the API Breaking Changes section, the entries provide a clear indication of significant changes that could affect users' existing implementations. It's essential to verify that these entries are comprehensive and include all necessary details for developers to understand the impact. Given the nature of these changes, it might be beneficial to include a brief explanation or rationale for each breaking change to help developers adapt their code. However, since this is not always standard practice in changelogs and might clutter the concise format, it's acceptable as is. Developers interested in the rationale can refer to the linked PRs for more context.
api/cosmos/slashing/v1beta1/slashing.pulsar.go (1)
- 1522-1588: The file defines protobuf-generated Go structures for
ValidatorSigningInfoandParamswithin the Cosmos SDK slashing module. These structures are crucial for managing validator penalties and parameters related to network security and liveness.
Correctness & Best Practices: The use of protobuf for defining these data structures ensures type safety and compatibility across different programming languages, which is essential for a blockchain framework like Cosmos SDK. The use of
google.protobuf.Timestampfor theJailedUntilfield inValidatorSigningInfoandgoogle.protobuf.Durationfor theDowntimeJailDurationfield inParamsis appropriate for representing time-based values.Documentation & Maintainability: The protobuf file from which this Go code is generated should include comprehensive documentation for each field. This documentation is crucial for developers interacting with the Cosmos SDK, ensuring clarity on the purpose and usage of each parameter within the slashing module.
Performance: The structures are optimized for performance in a distributed blockchain environment, with considerations for minimal overhead in serialization and deserialization processes. The use of scalar types like
int64for numerical values andboolfor flags ensures efficient storage and access patterns.Security: The design of these structures, particularly the
Tombstonedflag inValidatorSigningInfo, reflects the security considerations in penalizing malicious validators. It's important that the logic using these structures properly validates input data to prevent potential vulnerabilities, such as integer overflow or denial of service through resource exhaustion.Overall, the structures are well-designed for their purpose within the Cosmos SDK's slashing module. Future enhancements could include more detailed inline documentation in the protobuf definitions to improve developer experience and maintainability.
api/cosmos/accounts/interfaces/account_abstraction/v1/interface.pulsar.go (3)
- 2020-2023: The import statement for
google.golang.org/protobuf/types/known/anypbhas been removed. Ensure that this change does not affect the functionality of any message types that might have relied onAnytypes for polymorphic behavior.- 2022-2042: The message
MsgAuthenticateincludes fields for bundler, raw transaction, decoded transaction, and signer index. Ensure that the removal of theanypbimport does not impact the handling of theTxandTxRawfields, especially since these fields are crucial for transaction processing and authentication.- 2043-2067: The file structure and package declaration follow the expected conventions for Cosmos SDK modules. The use of protobuf for defining message types is consistent with Cosmos SDK practices. However, ensure that the removal of the
anypbimport does not impact any dynamic message handling that might have been intended in this or related modules.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Files selected for processing (11)
- api/cosmos/slashing/v1beta1/slashing.pulsar.go (3 hunks)
- proto/cosmos/slashing/v1beta1/slashing.proto (1 hunks)
- x/slashing/keeper/genesis_test.go (1 hunks)
- x/slashing/keeper/grpc_query_test.go (2 hunks)
- x/slashing/keeper/hooks.go (1 hunks)
- x/slashing/keeper/infractions.go (6 hunks)
- x/slashing/keeper/msg_server_test.go (4 hunks)
- x/slashing/keeper/signing_info_test.go (3 hunks)
- x/slashing/simulation/decoder_test.go (1 hunks)
- x/slashing/types/signing_info.go (1 hunks)
- x/slashing/types/slashing.pb.go (3 hunks)
Files not summarized due to errors (1)
- api/cosmos/slashing/v1beta1/slashing.pulsar.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (3)
- proto/cosmos/slashing/v1beta1/slashing.proto
- x/slashing/keeper/infractions.go
- x/slashing/types/slashing.pb.go
Additional comments: 14
x/slashing/types/signing_info.go (1)
- 9-9: The modification to remove the
indexOffsetparameter from theNewValidatorSigningInfofunction aligns with the PR's objective to deprecate theindex_offsetfield and streamline the slashing updates. This change simplifies the function signature and maintains the logic for creating a newValidatorSigningInfoinstance.x/slashing/simulation/decoder_test.go (1)
- 34-34: The update to the
TestDecodeStorefunction correctly reflects the removal of theindexOffsetparameter in theNewValidatorSigningInfocall. This change ensures that the test aligns with the updated function signature and supports the PR's objective of optimizing the slashing mechanism.x/slashing/keeper/genesis_test.go (2)
- 26-26: The update to the
TestExportAndInitGenesisfunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.- 28-28: The update to the
TestExportAndInitGenesisfunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.x/slashing/keeper/hooks.go (1)
- 39-44: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [34-34]
The update to the
AfterValidatorBondedfunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the function aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.x/slashing/keeper/grpc_query_test.go (2)
- 36-41: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [34-34]
The update to the
TestGRPCSigningInfofunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.
- 36-41: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [34-34]
The update to the
TestGRPCSigningInfosfunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.x/slashing/keeper/signing_info_test.go (3)
- 34-34: The update to the
TestValidatorSigningInfofunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.- 34-34: The update to the
TestValidatorMissedBlockBitmap_SmallWindowfunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.- 34-34: The update to the
TestPerformConsensusPubKeyUpdatefunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.x/slashing/keeper/msg_server_test.go (4)
- 224-224: The update to the
TestUnjailfunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.- 259-259: The update to the
TestUnjailfunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.- 294-294: The update to the
TestUnjailfunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.- 329-329: The update to the
TestUnjailfunction correctly reflects the changes made to theNewValidatorSigningInfofunction, ensuring that the test aligns with the updated function signature. This change supports the PR's objective of optimizing the slashing mechanism.
| // DEPRECATED: Index which is incremented every time a validator is bonded in a block and | ||
| // _may_ have signed a pre-commit or not. This in conjunction with the | ||
| // signed_blocks_window param determines the index in the missed block bitmap. | ||
| // | ||
| // Deprecated: Do not use. |
There was a problem hiding this comment.
Tip
Codebase Verification
The examination of index_offset references in both slashing.pb.go and slashing.pulsar.go confirms that while the field is marked as deprecated in the protobuf definition, it is still actively used in the codebase's logic. This includes conditional checks, assignments, and handling within slashing.pulsar.go, indicating that the deprecation has not been fully reflected in the removal or modification of its usage. Therefore, the initial review comment's concern about updating all references or usages of this field to reflect its deprecation is valid, and there are indeed issues that need to be addressed to ensure consistency and avoid potential runtime issues or confusion.
Analysis chain
The index_offset field in ValidatorSigningInfo is marked as deprecated with the protobuf option [(gogoproto.moretags) = "deprecated=true"]. This change aligns with the PR's objective to deprecate the index_offset field. However, it's crucial to ensure that all references or usages of this field in the codebase are also updated to reflect this deprecation, avoiding potential runtime issues or confusion.
To further analyze the usage of index_offset and ensure that its deprecation is properly handled in the codebase, we need to examine the specific references in both identified files. This will help us understand if there are any active usages that need to be addressed or if the references are part of the expected code structure, such as documentation or generated code.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of `index_offset` in the Go files to ensure it's not actively used or referenced.
rg --type go 'index_offset' --files-with-matches
Length of output: 127
Script:
#!/bin/bash
# Examine references to `index_offset` in `slashing.pb.go`
echo "References in slashing.pb.go:"
rg 'index_offset' x/slashing/types/slashing.pb.go --context 2
# Examine references to `index_offset` in `slashing.pulsar.go`
echo "References in slashing.pulsar.go:"
rg 'index_offset' api/cosmos/slashing/v1beta1/slashing.pulsar.go --context 2
Length of output: 3973
| @@ -1467,6 +1469,7 @@ | |||
| return 0 | |||
| } | |||
|
|
|||
| // Deprecated: Do not use. | |||
There was a problem hiding this comment.
The method GetIndexOffset is automatically generated due to the protobuf definition. Since index_offset is deprecated, consider adding a Go deprecation comment to this method to discourage its use in Go code, aligning with the protobuf deprecation.
- // Deprecated: Do not use.
+ //
+ // Deprecated: Do not use. This field is deprecated.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // Deprecated: Do not use. | |
| // | |
| // Deprecated: Do not use. This field is deprecated. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- tests/integration/evidence/keeper/infraction_test.go (1 hunks)
- tests/integration/slashing/keeper/keeper_test.go (6 hunks)
Additional comments: 7
tests/integration/evidence/keeper/infraction_test.go (1)
- 206-206: The modification to the
NewValidatorSigningInfocall in theTestHandleDoubleSignfunction removes an argument. This change aligns with the PR's objective to deprecate theindex_offsetfield inValidatorSigningInfo. Ensure that all references toNewValidatorSigningInfoacross the test suite and the main codebase are updated to match this new signature. Additionally, verify that the removal of this argument does not impact the logic of these tests, especially in scenarios where theindex_offsetmight have been used.tests/integration/slashing/keeper/keeper_test.go (6)
- 130-131: The removal of the
index_offsetparameter inNewValidatorSigningInfocalls within theinitFixturefunction aligns with the PR's objective of deprecating this field. However, ensure that the logic within these tests still accurately reflects the intended behavior of the slashing mechanism without relying on theindex_offset.- 252-252: The removal of the
index_offsetparameter in theTestHandleNewValidatorfunction is consistent with the deprecation strategy. It's crucial to verify that the test still effectively validates the new validator handling logic without this parameter.- 312-312: Similar to previous comments, the removal of the
index_offsetparameter in theTestHandleAlreadyJailedfunction is in line with the PR's goals. Ensure that the absence of this parameter does not compromise the test's ability to accurately assess the behavior of the slashing mechanism for already jailed validators.- 396-396: The removal of the
index_offsetparameter in theTestValidatorDippingInAndOutfunction is appropriate given the PR's objectives. Confirm that the test remains a valid assessment of the slashing mechanism's handling of validators dipping in and out of the validator set without relying on theindex_offset.- 457-457: The re-initialization of
ValidatorSigningInfoin theTestValidatorDippingInAndOutfunction, following the validator's jailing, correctly omits theindex_offsetparameter. This change should be carefully evaluated to ensure it does not affect the test's ability to simulate and verify the behavior of the slashing mechanism in scenarios involving validator jailing and rejoining.- 472-472: The re-initialization of
ValidatorSigningInfofor a validator rejoining and starting to sign again in theTestValidatorDippingInAndOutfunction, without theindex_offsetparameter, is consistent with the PR's deprecation of this field. It's important to ensure that this modification does not impact the test's effectiveness in evaluating the slashing mechanism's response to validators rejoining after a period of absence.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- tests/integration/slashing/abci_test.go (1 hunks)
- tests/sims/slashing/operations_test.go (1 hunks)
Additional comments: 2
tests/integration/slashing/abci_test.go (1)
- 93-93: The modification of the assertion for
info.IndexOffsetfrom1to0aligns with the PR's objective to optimize the slashing mechanism. This change likely reflects the updated logic for handling validator sign information, particularly after the deprecation of theindex_offsetfield. Ensure that this adjustment is consistent with the intended logic changes and that it does not inadvertently affect other aspects of the slashing process.tests/sims/slashing/operations_test.go (1)
- 170-170: The modification in the
NewValidatorSigningInfocall by removing theint64(3)argument is consistent with the PR's objectives to refine and optimize the slashing mechanism. This change likely reflects adjustments in the logic or data structure for handling validator sign information, particularly in light of the deprecation of theindex_offsetfield. Ensure that this simplification does not omit necessary validation checks or affect the test's ability to accurately simulate the unjailing process.
x/slashing/keeper/infractions.go
Outdated
| // The validator start height should get mapped to index 0, so we computed index as: | ||
| // (height - startHeight) % signedBlocksWindow | ||
| index := (height - signInfo.StartHeight) % signedBlocksWindow |
There was a problem hiding this comment.
This file / area is where the logic change is.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/slashing/keeper/infractions.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/slashing/keeper/infractions.go
|
Figured out the test failure -- the move to environment actually made two of the integration tests for slashing wrong, and led to the initial height in the tests being greater than the "perceived" block height. The code in the PR was right -- The issue was the integration tests. Can someone help me out with how to fix the tests? I'm searching as well, will reply if I figure it out :) We currently parse the block height as: The problematic test here is updating the block height as: WithBlockHeight does: Which doesn't get read by the new environment logic. If updating the test proves hard, I could just do something silly and make the state machine code make negative modulii wrap around. (Right now I just throw an error) |
|
Using withheaderinfo will solve the issue. |
|
Thank you!! Tests should now be fixed |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- tests/integration/slashing/keeper/keeper_test.go (11 hunks)
- x/slashing/keeper/infractions.go (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- tests/integration/slashing/keeper/keeper_test.go
- x/slashing/keeper/infractions.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- x/slashing/CHANGELOG.md (2 hunks)
- x/slashing/README.md (2 hunks)
- x/slashing/keeper/infractions.go (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/slashing/CHANGELOG.md
- x/slashing/keeper/infractions.go
Additional comments: 2
x/slashing/README.md (2)
- 215-219: The documentation update clearly explains the new method for calculating the index in the sliding window for validator liveness tracking. This change simplifies the logic by making the index calculation directly dependent on the block height and the validator's start height, which is a more intuitive approach than using an
index_offset. The explanation is concise and aligns well with the objective of improving the efficiency of the slashing module.- 239-240: The comments within the code snippet provide helpful clarification on how the index is calculated relative to the validator's signing information. It's good to see that the documentation emphasizes the use of the default signing info if not present, which ensures that new validators are correctly handled. This detail is crucial for understanding how the system initializes and updates the
MissedBlocksBitArrayandMissedBlocksCounterfor validators.
| // have signed. We will also use the 0-value default signing info if not present. | ||
| // The index is in the range [0, SignedBlocksWindow) |
There was a problem hiding this comment.
Note I deleted the comment
except for start height. because I think this part of the comment is inaccurate in the current code. There is no logic anywhere here that says something to the effect if startHeight = 0 { startHeight = height }, nor any tests or logic that assume this.
Somewhat of a driveby change, but figured I should edit it as I'm making another thing depend on StartHeight.
| // NOTE: There is subtle different behavior between genesis validators and non-genesis validators. | ||
| // A genesis validator will start at index 0, whereas a non-genesis validator's startHeight will be the block | ||
| // they bonded on, but the first block they vote on will be one later. (And thus their first vote is at index 1) |
There was a problem hiding this comment.
This is safe / should pose no issues imo.
We only look for liveness slashes after a full SignedBlockWindow blocks have passed. This has always granted one extra block for genesis validators, and exactly SignedBlockWindow number of blocks for non-genesis validators, before liveness slashing is looked at.
During a migration, this would cause the liveness tracking for the non-genesis validators to "skip" one block ahead. E.g. suppose the upgrade is at block N.
Pre-upgrade (block N-1) is at index 5. Post-upgrade (block N), my liveness index is 7 (not 6). So we "inherit" one old block in the liveness tracking for this window, but it gets fully fixed a SignedBlockWindow later. I really don't think this is an issue, and for genesis validators the index is preserved correctly.
If anything, I think this is generally lowering the complexity of the logic difference for the genesis/non-genesis discrepancy. (as now they will share the exact same indices)
facundomedica
deleted the
dev/make_slashing_not_write_signinfo_every_block
branch
) (#543) * perf!: Make slashing not write sign info every block (SDK: cosmos#19458) cosmos#19458 * change NewSignInfo API * Bring back String() change * Fix test * Another test fix that didn't get committed * merge conflict --------- Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
…) (#543) * perf!: Make slashing not write sign info every block (SDK: cosmos#19458) cosmos#19458 * change NewSignInfo API * Bring back String() change * Fix test * Another test fix that didn't get committed * merge conflict --------- Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Description
Part of: #19457
This PR changes slashing, to not update the sign info for every single validator, every single block. Previously we were updating it if:
This also deprecates the proto field. This PR is in draft until I get proto compiling on my new laptop 😅
But it won't even require any migration! Trying to think of what tests to add, but I think it should be covered by the tests in staking / simulation already. Please make any suggestions!
Note that Unjailing sets a new SigningInfo, as that gets called in this hook: https://github.com/cosmos/cosmos-sdk/blob/main/x/slashing/keeper/hooks.go#L39-L47 . Where the old convention matches the new code! Reset StartHeight, and make that index 0!
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
JailUntilandTombstonemethods to improve error handling.Refactor
index_offsetfield in favor of enhanced tracking and performance optimizations.Chores
CHANGELOG.mdto reflect new functionalities and improvements.