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.

Sign up for GitHub

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

Jump to bottom

querying empty signal tally causes index panic #4007

Closed
cmwaters opened this issue Oct 24, 2024 · 4 comments · Fixed by #4045
Closed

querying empty signal tally causes index panic #4007

cmwaters opened this issue Oct 24, 2024 · 4 comments · Fixed by #4045
Assignees
@rootulp
Labels
bug Something isn't working priority:low optional label to track the relative priority of planned items WS: Maintenance 🔧 includes bugs, refactors, flakes, and tech debt etc

Comments

@cmwaters
Copy link
Contributor

Found by using the following command on arabica:

❯ celestia-appd query signal tally 3 --node https://rpc.celestia-arabica-11.com:443
Error: rpc error: code = Unknown desc = runtime error: index out of range [7] with length 7: panic
@evan-forbes evan-forbes added bug Something isn't working WS: Maintenance 🔧 includes bugs, refactors, flakes, and tech debt etc priority:low optional label to track the relative priority of planned items and removed needs:triage labels Nov 11, 2024
@rootulp rootulp self-assigned this Nov 18, 2024
@rootulp
Copy link
Collaborator

rootulp commented Nov 18, 2024 •
edited
Loading

hmm, I can't repro with that exact command or a few variants of that command:

celestia-appd query signal tally 3 --node https://celestia-mainnet-rpc.itrocket.net:443
celestia-appd query signal tally 4 --node https://rpc.celestia-arabica-11.com:443
celestia-appd query signal tally 5 --node https://rpc.celestia-arabica-11.com:443

I think we'll need to write unit tests against the RPC / gRPC endpoint to try and repro

@rootulp

This comment was marked as outdated.

Sign in to view
@rootulp
Copy link
Collaborator

rootulp commented Nov 21, 2024 •
edited
Loading

I reproed this by running single-node.sh from #4041 and then ./scripts/upgrade-to-v3.sh

# Query during app version 1
$ celestia-appd query signal tally 3
Error: rpc error: code = Unknown desc = kv store with key KVStoreKey{0x14001209a90, signal} has not been registered in stores: panic

# Query during app version 2
$ celestia-appd query signal tally 3
threshold_power: "4167"
total_voting_power: "5000"
voting_power: "0"

# Query after validator signals for v3
$ celestia-appd query signal tally 3
threshold_power: "4167"
total_voting_power: "5000"
voting_power: "5000"

# Query after the try upgrade but before v3 activation height
$ celestia-appd query signal tally 3
Error: rpc error: code = Unknown desc = runtime error: index out of range [7] with length 6: panic

@rootulp
Copy link
Collaborator

rootulp commented Nov 21, 2024

The upgrade key is getting iterated over when calculating the tally. I added a unit test and fix.

@rootulp rootulp mentioned this issue Nov 21, 2024
Merged
mergify bot pushed a commit that referenced this issue Nov 25, 2024
@rootulp @mergify
fix: query signal tally after successful try upgrade (#4045)
ba6bef3 
Closes #4007

## Testing

Using the updated single-node.sh script (see other PR), I can query the
version tally even after a successful try upgrade.

```
$ ./scripts/upgrade-to-v3.sh

$ celestia-appd query signal tally 3
threshold_power: "4167"
total_voting_power: "5000"
voting_power: "5000"
```

(cherry picked from commit 0bfd074)
@mergify mergify bot mentioned this issue Nov 25, 2024
Merged
evan-forbes pushed a commit that referenced this issue Nov 25, 2024
@mergify @rootulp
fix: query signal tally after successful try upgrade (backport #4045)…
c7bc385 
… (#4052)

Closes #4007

## Testing

Using the updated single-node.sh script (see other PR), I can query the
version tally even after a successful try upgrade.

```
$ ./scripts/upgrade-to-v3.sh

$ celestia-appd query signal tally 3
threshold_power: "4167"
total_voting_power: "5000"
voting_power: "5000"
```<hr>This is an automatic backport of pull request #4045 done by
[Mergify](https://mergify.com).

Co-authored-by: Rootul P <[email protected]>
rach-id pushed a commit that referenced this issue Nov 26, 2024
@rootulp @rach-id
fix: query signal tally after successful try upgrade (#4045)
2aee70a 
Closes #4007

## Testing

Using the updated single-node.sh script (see other PR), I can query the
version tally even after a successful try upgrade.

```
$ ./scripts/upgrade-to-v3.sh

$ celestia-appd query signal tally 3
threshold_power: "4167"
total_voting_power: "5000"
voting_power: "5000"
```

(cherry picked from commit 0bfd074)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rootulp rootulp

Labels
bug Something isn't working priority:low optional label to track the relative priority of planned items WS: Maintenance 🔧 includes bugs, refactors, flakes, and tech debt etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: query signal tally after successful try upgrade rootulp/celestia-app
3 participants
@rootulp @cmwaters @evan-forbes