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

Replace hash maps and sets with indexmap equivalents #2685

Merged
merged 5 commits into from Apr 12, 2024
Merged

Replace hash maps and sets with indexmap equivalents #2685

merged 5 commits into from Apr 12, 2024

Conversation

sug0
Copy link
Contributor

@sug0 sug0 commented Feb 21, 2024 •
edited

Describe your changes

Closes #2576

Indicate on which release or other PRs this topic is based on

Based on v0.31.6

Indexmap diff: https://github.com/heliaxdev/indexmap/compare/4df99d818369b437e9e09e204832071fbd5b39dd..2.2.4-heliax-1

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@sug0 sug0 added bug Something isn't working security breaking-change labels Feb 21, 2024
@sug0
Copy link
Contributor Author

sug0 commented Feb 21, 2024

might need to rebuild wasms for tests

@sug0
Copy link
Contributor Author

sug0 commented Feb 21, 2024 •
edited

TODO: update indexmap
https://github.com/heliaxdev/indexmap/releases/tag/2.2.3-heliax-3

@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from 6821eef to cd1ed75 Compare February 22, 2024 09:49
@codecov Codecov
Copy link

codecov bot commented Feb 22, 2024 •
edited

Codecov Report

Attention: Patch coverage is 72.41379% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 53.95%. Comparing base (2535c9c) to head (df1038d).
Report is 161 commits behind head on main.

Files Patch % Lines
crates/sdk/src/wallet/mod.rs 0.00% 4 Missing ⚠️
...tes/namada/src/vm/wasm/compilation_cache/common.rs 50.00% 2 Missing ⚠️
crates/sdk/src/events/mod.rs 0.00% 1 Missing ⚠️
crates/sdk/src/rpc.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2685      +/-   ##
==========================================
- Coverage   53.95%   53.95%   -0.01%     
==========================================
  Files         308      308              
  Lines      100018   100021       +3     
==========================================
+ Hits        53967    53968       +1     
- Misses      46051    46053       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cwgoes
cwgoes previously approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK modulo CI

sug0 added a commit that referenced this pull request Feb 22, 2024
@sug0
Changelog for #2685
3120577
@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from cd1ed75 to 3120577 Compare February 22, 2024 10:51
@sug0 sug0 marked this pull request as ready for review February 22, 2024 10:51
@sug0
Copy link
Contributor Author

sug0 commented Feb 22, 2024

e2e tests in CI passed without rebuilding wasms for tests, but it might still be wise to rebuild them anyway.

batconjurer
batconjurer previously approved these changes Feb 22, 2024
View reviewed changes
@tzemanovic
Copy link
Member

I've added f1e4d72 to prevent from using them again

tzemanovic
tzemanovic previously approved these changes Feb 22, 2024
View reviewed changes
sug0 added a commit that referenced this pull request Feb 22, 2024
@sug0
Changelog for #2685
b050052
@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from f1e4d72 to b050052 Compare February 22, 2024 14:16
sug0 added a commit that referenced this pull request Feb 26, 2024
@sug0
Changelog for #2685
d9cb961
@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from b050052 to d9cb961 Compare February 26, 2024 10:26
@sug0
Copy link
Contributor Author

sug0 commented Feb 26, 2024 •
edited

updated the indexmap dep
rolled back to the previous tag, the new deserialization code had OOM exploits

sug0 added a commit that referenced this pull request Feb 26, 2024
@sug0
Changelog for #2685
db5b83e
@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from d9cb961 to db5b83e Compare February 26, 2024 12:39
@sug0 sug0 marked this pull request as draft February 27, 2024 10:01
sug0 added a commit that referenced this pull request Feb 27, 2024
@sug0
Changelog for #2685
5734a79
@sug0
Copy link
Contributor Author

sug0 commented Feb 27, 2024

rebased on v0.31.6 (kept borsh schemas)

@sug0 sug0 marked this pull request as draft February 28, 2024 16:03
@sug0
Copy link
Contributor Author

sug0 commented Feb 28, 2024 •
edited

TODO: rebase on v0.31.8

sug0 added a commit that referenced this pull request Feb 29, 2024
@sug0
Changelog for #2685
c310ae6
@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from 5734a79 to c310ae6 Compare February 29, 2024 10:56
@sug0 sug0 marked this pull request as ready for review February 29, 2024 10:58
sug0 added a commit that referenced this pull request Feb 29, 2024
@sug0
Changelog for #2685
75446b7
@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from c310ae6 to 75446b7 Compare February 29, 2024 11:00
sug0 added a commit that referenced this pull request Feb 29, 2024
@sug0
Changelog for #2685
65611bf
@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from 75446b7 to 65611bf Compare February 29, 2024 11:20
sug0 added a commit that referenced this pull request Feb 29, 2024
@sug0
Changelog for #2685
76ebb15
@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from 65611bf to 76ebb15 Compare February 29, 2024 11:22
@sug0 sug0 force-pushed the tiago/replace-hash-data-structs branch from 76ebb15 to df1038d Compare February 29, 2024 13:04
@sug0
Copy link
Contributor Author

sug0 commented Feb 29, 2024

rebased on v0.31.8 and updated indexmap to 2.2.4-heliax-1 (the new upstream version prevents OOM errors in the serde::Deserialize implementation)

@brentstone brentstone mentioned this pull request Mar 7, 2024
Closed
10 tasks
@tzemanovic tzemanovic mentioned this pull request Mar 18, 2024
Closed
tzemanovic added a commit that referenced this pull request Mar 19, 2024
@tzemanovic
Merge branch 'tiago/replace-hash-data-structs' (#2685)
210ab20 
* origin/tiago/replace-hash-data-structs:
  Changelog for #2685
  add clippy config
  Replace std hash map and set collections with indexmap alternatives
  Re-export index map and set from core
  Add `indexmap` dep to Cargo manifest
tzemanovic added a commit that referenced this pull request Mar 19, 2024
@tzemanovic
fixup! Merge branch 'tiago/replace-hash-data-structs' (#2685)
7119185
@brentstone brentstone added this to the Phase 1: mainnet genesis milestone Apr 10, 2024
tzemanovic added a commit that referenced this pull request Apr 10, 2024
@tzemanovic
Merge branch 'tiago/replace-hash-data-structs' (#2685)
c1f7220 
* origin/tiago/replace-hash-data-structs:
  Changelog for #2685
  add clippy config
  Replace std hash map and set collections with indexmap alternatives
  Re-export index map and set from core
  Add `indexmap` dep to Cargo manifest

# Conflicts:
#	crates/apps/src/lib/client/rpc.rs
#	crates/apps/src/lib/node/ledger/shell/init_chain.rs
#	crates/core/src/ibc.rs
#	crates/namada/src/ledger/protocol/mod.rs
@tzemanovic tzemanovic merged commit 63b3718 into main Apr 12, 2024
16 of 17 checks passed
@tzemanovic tzemanovic deleted the tiago/replace-hash-data-structs branch April 12, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzemanovic tzemanovic tzemanovic approved these changes

@batconjurer batconjurer batconjurer left review comments

@cwgoes cwgoes cwgoes left review comments

Assignees
No one assigned
Labels
breaking-change bug Something isn't working merged to draft ready for draft security
Projects
None yet
Milestone
Phase 1: mainnet genesis
Development

Successfully merging this pull request may close these issues.

Audit for all uses of HashSet in the codebase
5 participants
@sug0 @tzemanovic @batconjurer @cwgoes @brentstone