Drop the `ahash` dependency #2891

TheBlueMatt · 2024-02-12T22:48:28Z

tkaitchuck/aHash#196 bumped the MSRV of
ahash in a patch release, which makes it rather difficult for us
to have it as a dependency.

Further, it seems that ahash hasn't been particularly robust in
the past, notably
tkaitchuck/aHash#163 and
tkaitchuck/aHash#166.

Luckily, core provides SipHasher even on no-std (sadly its
SipHash-2-4 unlike the SipHash-1-3 used by the DefaultHasher in
std). Thus, we drop the ahash dependency entirely here and
simply wrap SipHasher for our no-std HashMaps.

coderabbitai · 2024-02-12T22:48:44Z

Walkthrough

The series of changes focus on enhancing the hash table implementations across the project, shifting between std, hashbrown, and new hashing strategies to optimize performance and security. A notable introduction is the handling of hash table selection and configuration via feature flags, alongside adjustments for HashDoS attack protection. The restructuring also extends to testing and fuzzing environments, ensuring consistency and the effective use of hash tables throughout the codebase.

Changes

Files	Summary
`.lightning/src/lib.rs` `.lightning-invoice/src/lib.rs`	Restructured hash table handling and simplified module imports.
`.lightning/src/util/hash_tables.rs` `.lightning/src/util/mod.rs`	Introduced `hash_tables.rs` for hash table re-exports and added to public scope.
`.ci/check-cfg-flags.py` `.github/workflows/build.yml`	Renamed "ahash" feature to "possiblyrandom" and adjusted feature flags in workflow.
`.fuzz/src/chanmon_consistency.rs` `.fuzz/src/full_stack.rs` `.fuzz/src/indexedmap.rs` `.fuzz/src/router.rs`	Replaced data structures with `util::hash_tables` module and adjusted usage.
`.lightning-invoice/src/utils.rs`	Switched from `HashMap` to `BTreeMap` for channel sorting and filtering.
`.possiblyrandom/src/lib.rs`	Added functionality for generating possibly-random data.

🐇✨
Changes abound, through the code they weave,
Hash tables shift, in magic we believe.
With flags aflutter, in randomness we trust,
Through bytes and bits, in CodeRabbit we must.
Let's hop to it, with each line we adjust!
🌟🐾

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository from git and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 0c2a715 and ba58745.

Files ignored due to path filters (4)

Cargo.toml is excluded by: !**/*.toml
bench/Cargo.toml is excluded by: !**/*.toml
lightning/Cargo.toml is excluded by: !**/*.toml
possiblyrandom/Cargo.toml is excluded by: !**/*.toml

Files selected for processing (4)

lightning/src/lib.rs (1 hunks)
lightning/src/util/hash_tables.rs (1 hunks)
lightning/src/util/mod.rs (1 hunks)
possiblyrandom/src/lib.rs (1 hunks)

Additional comments: 5

lightning/src/util/mod.rs (1)

35-35: LGTM!

possiblyrandom/src/lib.rs (2)

21-38: LGTM!

34-38: LGTM!

lightning/src/lib.rs (1)

173-173: SKIP: No specific code changes annotated for review.

lightning/src/util/hash_tables.rs (1)

7-164: LGTM!

codecov-commenter · 2024-02-12T22:57:14Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (73da722) 89.13% compared to head (5243ef9) 89.39%.
Report is 12 commits behind head on main.

❗ Current head 5243ef9 differs from pull request most recent head eecd2cd. Consider uploading reports for the commit eecd2cd to get more accurate results

Files	Patch %	Lines
possiblyrandom/src/lib.rs	0.00%	8 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2891      +/-   ##
==========================================
+ Coverage   89.13%   89.39%   +0.26%     
==========================================
  Files         115      117       +2     
  Lines       94179    96569    +2390     
  Branches    94179    96569    +2390     
==========================================
+ Hits        83944    86327    +2383     
- Misses       7761     7816      +55     
+ Partials     2474     2426      -48

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

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 0995de7 and dcf6d5c.

Files ignored due to path filters (4)

Cargo.toml is excluded by: !**/*.toml
bench/Cargo.toml is excluded by: !**/*.toml
lightning/Cargo.toml is excluded by: !**/*.toml
possiblyrandom/Cargo.toml is excluded by: !**/*.toml

Files selected for processing (5)

ci/check-cfg-flags.py (1 hunks)
lightning/src/lib.rs (1 hunks)
lightning/src/util/hash_tables.rs (1 hunks)
lightning/src/util/mod.rs (1 hunks)
possiblyrandom/src/lib.rs (1 hunks)

Files skipped from review as they are similar to previous changes (4)

lightning/src/lib.rs
lightning/src/util/hash_tables.rs
lightning/src/util/mod.rs
possiblyrandom/src/lib.rs

Additional comments: 2

ci/check-cfg-flags.py (2)

16-17: Ensure the "possiblyrandom" feature is correctly integrated into other parts of the project, as this change indicates its significance in feature management.

18-18: The addition of the "getrandom" feature should be verified for correct usage across the project, especially in contexts where OS randomness is leveraged.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 0995de7 and 852d1d8.

Files ignored due to path filters (6)

Cargo.toml is excluded by: !**/*.toml
bench/Cargo.toml is excluded by: !**/*.toml
fuzz/Cargo.toml is excluded by: !**/*.toml
lightning-invoice/Cargo.toml is excluded by: !**/*.toml
lightning/Cargo.toml is excluded by: !**/*.toml
possiblyrandom/Cargo.toml is excluded by: !**/*.toml

Files selected for processing (12)

.github/workflows/build.yml (1 hunks)
ci/check-cfg-flags.py (1 hunks)
fuzz/src/chanmon_consistency.rs (8 hunks)
fuzz/src/full_stack.rs (6 hunks)
fuzz/src/indexedmap.rs (2 hunks)
fuzz/src/router.rs (8 hunks)
lightning-invoice/src/lib.rs (1 hunks)
lightning-invoice/src/utils.rs (4 hunks)
lightning/src/lib.rs (1 hunks)
lightning/src/util/hash_tables.rs (1 hunks)
lightning/src/util/mod.rs (1 hunks)
possiblyrandom/src/lib.rs (1 hunks)

Files skipped from review as they are similar to previous changes (12)

.github/workflows/build.yml
ci/check-cfg-flags.py
fuzz/src/chanmon_consistency.rs
fuzz/src/full_stack.rs
fuzz/src/indexedmap.rs
fuzz/src/router.rs
lightning-invoice/src/lib.rs
lightning-invoice/src/utils.rs
lightning/src/lib.rs
lightning/src/util/hash_tables.rs
lightning/src/util/mod.rs
possiblyrandom/src/lib.rs

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between f98a652 and d3b0af0.

Files ignored due to path filters (7)

Cargo.toml is excluded by: !**/*.toml
bench/Cargo.toml is excluded by: !**/*.toml
fuzz/Cargo.toml is excluded by: !**/*.toml
lightning-invoice/Cargo.toml is excluded by: !**/*.toml
lightning/Cargo.toml is excluded by: !**/*.toml
no-std-check/Cargo.toml is excluded by: !**/*.toml
possiblyrandom/Cargo.toml is excluded by: !**/*.toml

Files selected for processing (12)

.github/workflows/build.yml (1 hunks)
ci/check-cfg-flags.py (1 hunks)
fuzz/src/chanmon_consistency.rs (8 hunks)
fuzz/src/full_stack.rs (6 hunks)
fuzz/src/indexedmap.rs (2 hunks)
fuzz/src/router.rs (8 hunks)
lightning-invoice/src/lib.rs (1 hunks)
lightning-invoice/src/utils.rs (4 hunks)
lightning/src/lib.rs (1 hunks)
lightning/src/util/hash_tables.rs (1 hunks)
lightning/src/util/mod.rs (1 hunks)
possiblyrandom/src/lib.rs (1 hunks)

Files skipped from review as they are similar to previous changes (12)

.github/workflows/build.yml
ci/check-cfg-flags.py
fuzz/src/chanmon_consistency.rs
fuzz/src/full_stack.rs
fuzz/src/indexedmap.rs
fuzz/src/router.rs
lightning-invoice/src/lib.rs
lightning-invoice/src/utils.rs
lightning/src/lib.rs
lightning/src/util/hash_tables.rs
lightning/src/util/mod.rs
possiblyrandom/src/lib.rs

TheBlueMatt · 2024-02-14T21:25:10Z

Note the one failing CI job here is an "out of disk space" issue...

wpaulino

LGTM

fuzz/src/indexedmap.rs

possiblyrandom/Cargo.toml

possiblyrandom/src/lib.rs

jkczyz · 2024-02-16T19:24:36Z

fuzz/src/chanmon_consistency.rs

-			latest_monitors: Mutex::new(HashMap::new()),
+			latest_monitors: Mutex::new(new_hash_map()),


When should we use functions like new_hash_map? I see it's also used in some non-fuzzing code currently.

Always, when we upgraded hashbrown a few weeks ago we had to start using it as we now can't use HashMap::new directly (outside of std).

jkczyz

LGTM. Please squash.

In the next commit we'll drop the `ahash` dependency in favor of directly calling `getrandom` to seed our hash tables. However, we'd like to depend on `getrandom` only on certain platforms *and* only when certain features (no-std) are set. This introduces an indirection crate to do so, allowing us to depend on it only when `no-std` is set but only depending on `getrandom` on platforms which it supports.

tkaitchuck/aHash#196 bumped the MSRV of `ahash` in a patch release, which makes it rather difficult for us to have it as a dependency. Further, it seems that `ahash` hasn't been particularly robust in the past, notably tkaitchuck/aHash#163 and tkaitchuck/aHash#166. Luckily, `core` provides `SipHasher` even on no-std (sadly its SipHash-2-4 unlike the SipHash-1-3 used by the `DefaultHasher` in `std`). Thus, we drop the `ahash` dependency entirely here and simply wrap `SipHasher` for our `no-std` HashMaps.

TheBlueMatt · 2024-02-16T20:35:02Z

Squashed without further changes.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e32020c and eecd2cd.

Files ignored due to path filters (7)

Cargo.toml is excluded by: !**/*.toml
bench/Cargo.toml is excluded by: !**/*.toml
fuzz/Cargo.toml is excluded by: !**/*.toml
lightning-invoice/Cargo.toml is excluded by: !**/*.toml
lightning/Cargo.toml is excluded by: !**/*.toml
no-std-check/Cargo.toml is excluded by: !**/*.toml
possiblyrandom/Cargo.toml is excluded by: !**/*.toml

Files selected for processing (12)

.github/workflows/build.yml (1 hunks)
ci/check-cfg-flags.py (1 hunks)
fuzz/src/chanmon_consistency.rs (8 hunks)
fuzz/src/full_stack.rs (6 hunks)
fuzz/src/indexedmap.rs (2 hunks)
fuzz/src/router.rs (8 hunks)
lightning-invoice/src/lib.rs (1 hunks)
lightning-invoice/src/utils.rs (4 hunks)
lightning/src/lib.rs (1 hunks)
lightning/src/util/hash_tables.rs (1 hunks)
lightning/src/util/mod.rs (1 hunks)
possiblyrandom/src/lib.rs (1 hunks)

Files skipped from review as they are similar to previous changes (12)

.github/workflows/build.yml
ci/check-cfg-flags.py
fuzz/src/chanmon_consistency.rs
fuzz/src/full_stack.rs
fuzz/src/indexedmap.rs
fuzz/src/router.rs
lightning-invoice/src/lib.rs
lightning-invoice/src/utils.rs
lightning/src/lib.rs
lightning/src/util/hash_tables.rs
lightning/src/util/mod.rs
possiblyrandom/src/lib.rs

tnull · 2024-02-20T08:10:52Z

FWIW, they now reverted their MSRV bump with the just-released v0.8.9 (cf. tkaitchuck/aHash#208).
So if we'd ever see any notable downsides switching away (e.g., performance regressions) we could consider it again in the future.

TheBlueMatt · 2024-02-20T18:32:44Z

It is a performance regression (ahash uses AES on platforms with AES intrinsics, and a very weak hash function otherwise, and we replaced it with SipHash-2-4), but if we get around to fixing it we should consider fixing it for std as well as no-std (as std always uses the default, which is SipHash-1-3). The particularly weak hash function fallback of ahash is not a super great fallback, though, doubly so considering in most cases that's what we were using ahash for, since it was no-std only.

Ultimately the only place the performance matters that much is in routing, and we might consider something weaker more generally there.

TheBlueMatt mentioned this pull request Feb 12, 2024

Pin ahash version that supports LDK's MSRV. #2890

Closed

coderabbitai bot reviewed Feb 12, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-02-no-ahash branch 2 times, most recently from 02b187f to 0ce96c6 Compare February 12, 2024 22:51

TheBlueMatt force-pushed the 2024-02-no-ahash branch 2 times, most recently from 8ef043d to dcf6d5c Compare February 13, 2024 17:56

coderabbitai bot reviewed Feb 13, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-02-no-ahash branch 4 times, most recently from 1b4382a to 852d1d8 Compare February 13, 2024 19:48

coderabbitai bot reviewed Feb 13, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-02-no-ahash branch from 852d1d8 to d3b0af0 Compare February 13, 2024 21:26

coderabbitai bot reviewed Feb 13, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-02-no-ahash branch from d3b0af0 to d887c4f Compare February 13, 2024 21:41

wpaulino reviewed Feb 14, 2024

View reviewed changes

fuzz/src/indexedmap.rs Show resolved Hide resolved

wpaulino previously approved these changes Feb 15, 2024

View reviewed changes

wpaulino mentioned this pull request Feb 16, 2024

Preliminary refactoring & structure for dual-funded channels #2770

Merged

jkczyz reviewed Feb 16, 2024

View reviewed changes

TheBlueMatt dismissed wpaulino’s stale review via 5243ef9 February 16, 2024 19:52

TheBlueMatt force-pushed the 2024-02-no-ahash branch from d887c4f to 5243ef9 Compare February 16, 2024 19:52

jkczyz reviewed Feb 16, 2024

View reviewed changes

TheBlueMatt added 3 commits February 16, 2024 20:34

Drop lightning-invoice dependency on hashbrown`

eecd2cd

TheBlueMatt force-pushed the 2024-02-no-ahash branch from 5243ef9 to eecd2cd Compare February 16, 2024 20:34

coderabbitai bot reviewed Feb 16, 2024

View reviewed changes

jkczyz approved these changes Feb 16, 2024

View reviewed changes

wpaulino approved these changes Feb 16, 2024

View reviewed changes

dunxen approved these changes Feb 19, 2024

View reviewed changes

TheBlueMatt merged commit cd84757 into lightningdevkit:main Feb 19, 2024
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop the `ahash` dependency #2891

Drop the `ahash` dependency #2891

TheBlueMatt commented Feb 12, 2024 •

edited

coderabbitai bot commented Feb 12, 2024 •

edited

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

coderabbitai bot left a comment

codecov-commenter commented Feb 12, 2024 •

edited

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

TheBlueMatt commented Feb 14, 2024

wpaulino left a comment

jkczyz Feb 16, 2024

TheBlueMatt Feb 16, 2024

jkczyz left a comment

TheBlueMatt commented Feb 16, 2024

coderabbitai bot left a comment

tnull commented Feb 20, 2024

TheBlueMatt commented Feb 20, 2024 •

edited

		latest_monitors: Mutex::new(HashMap::new()),
		latest_monitors: Mutex::new(new_hash_map()),

Drop the ahash dependency #2891

Drop the ahash dependency #2891

Conversation

TheBlueMatt commented Feb 12, 2024 • edited

coderabbitai bot commented Feb 12, 2024 • edited

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 12, 2024 • edited

Codecov Report

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 14, 2024

wpaulino left a comment

Choose a reason for hiding this comment

jkczyz Feb 16, 2024

Choose a reason for hiding this comment

TheBlueMatt Feb 16, 2024

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 16, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

tnull commented Feb 20, 2024

TheBlueMatt commented Feb 20, 2024 • edited

Drop the `ahash` dependency #2891

Drop the `ahash` dependency #2891

TheBlueMatt commented Feb 12, 2024 •

edited

coderabbitai bot commented Feb 12, 2024 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)

codecov-commenter commented Feb 12, 2024 •

edited

TheBlueMatt commented Feb 20, 2024 •

edited