Use hashbrown replacements for std equivalents #947

GeneFerneau · 2021-06-10T00:31:24Z

No description provided.

GeneFerneau · 2021-06-10T00:32:59Z

lightning/Cargo.toml

@@ -29,6 +29,7 @@ unstable = []
 [dependencies]
 bitcoin = "0.26"

+hashbrown = { version = "0.9" }


Current hashbrown is 0.11.2, can bump to 0.11, if MSRV 1.49 is OK

Can also feature-gate inside the prelude module, too.

TheBlueMatt · 2021-06-10T00:38:42Z

Can we make this optional instead with a "hashbrown" feature (even an implicit one where you just make "hashbrown" an optional dependency)? Then we can make no-std depend on it.

codecov · 2021-06-10T03:44:31Z

Codecov Report

Merging #947 (da7a851) into main (4d1c1a3) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #947      +/-   ##
==========================================
+ Coverage   90.56%   90.58%   +0.01%     
==========================================
  Files          60       60              
  Lines       30423    30423              
==========================================
+ Hits        27553    27559       +6     
+ Misses       2870     2864       -6

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	`95.80% <ø> (ø)`
lightning/src/chain/channelmonitor.rs	`90.76% <ø> (ø)`
lightning/src/chain/keysinterface.rs	`95.41% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`94.14% <ø> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.78% <ø> (ø)`
lightning/src/ln/channelmanager.rs	`83.85% <ø> (ø)`
lightning/src/ln/functional_test_utils.rs	`94.81% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.22% <ø> (+0.09%)`	⬆️
lightning/src/ln/peer_handler.rs	`44.30% <ø> (ø)`
lightning/src/ln/reorg_tests.rs	`99.62% <ø> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d1c1a3...da7a851. Read the comment docs.

devrandom

ACK other than nits

lightning/src/routing/router.rs

lightning/src/lib.rs

lightning/Cargo.toml

TheBlueMatt · 2021-06-10T16:00:58Z

lightning/src/lib.rs

I think we should directly check for the hashbrown feature. Ideally we'd be able to use hashbrown with std or no_std, but require it for no_std. Specifically, I'd like to use hashbrown with std in the fuzzers, as I can disable the default randomization of keys while fuzzing.

Hrm, I tried directly naming a hashbrown feature, but I think it collides with the dependency name. Maybe I'm doing something wrong?

Huh, I thought if you don't even write out the feature and just make the dependency optional you can use it as a feature in cfg.

Ok, maybe that's what I was doing wrong. I tried writing out hashbrown = ["hashbrown"] as a feature. Will try what you described.

TheBlueMatt · 2021-06-11T02:15:53Z

ci/check-compiles.sh

@@ -6,3 +6,4 @@ cargo check
 cargo doc
 cargo doc --document-private-items
 cd fuzz && cargo check --features=stdin_fuzz
+cd ../lightning && cargo check --no-default-features --features=no_std


In .github/workflows/build.yml can you also add a cd lightning && cargo test --verbose --color always --features hashbrown to run the test suite with hashbrown + std?

Which builds should that run under, or do you want a new build target?

Uhhmmmmm, yes? I don't think it matters too much, maybe in the existing ones is simplest, given it's ~one line?

TheBlueMatt · 2021-06-11T02:18:51Z

lightning/src/routing/router.rs

 	pub(super) fn random_init_seed() -> u64 {
 		// Because the default HashMap in std pulls OS randomness, we can use it as a (bad) RNG.
 		use core::hash::{BuildHasher, Hasher};
 		let seed = std::collections::hash_map::RandomState::new().build_hasher().finish();
 		println!("Using seed of {}", seed);
 		seed
 	}
+    #[cfg(not(feature = "no_std"))]


nit: indentation with tabs.

agh! yeah, my editor defaults to indenting with spaces. Would you be open to adding a commit hook to automatically format?

If there's an easy way to just swap spaces for tabs, sure (I don't think there is, though?), but we've tried rustfmt a few times and it makes tons of code completely unreadable. It's desire to lay out all your code completely vertically means you lose a ton of context when reviewing more often than not. There's a few issues open upstream to make it more sensible, but I don't believe there's been much progress on any of them.

Ok, I won't worry about it then. Thought there might be a simple fix.

TheBlueMatt · 2021-06-12T23:09:26Z

.github/workflows/build.yml

@@ -56,6 +56,8 @@ jobs:
          cargo build --verbose --color always --features rpc-client
          cargo build --verbose --color always --features rpc-client,rest-client
          cargo build --verbose --color always --features rpc-client,rest-client,tokio
+          cd ../lightning
+          cargo build --verbose --color always --features hashbrown


We should also test with hashbrown in the "! matrix.build-net-tokio" block immediately above this one.

TheBlueMatt · 2021-06-12T23:10:44Z

.github/workflows/build.yml

@@ -92,6 +98,8 @@ jobs:
          RUSTFLAGS="-C link-dead-code" cargo test --verbose --color always --features rpc-client
          RUSTFLAGS="-C link-dead-code" cargo test --verbose --color always --features rpc-client,rest-client
          RUSTFLAGS="-C link-dead-code" cargo test --verbose --color always --features rpc-client,rest-client,tokio
+          cd ../lightning
+          RUSTFLAGS="-C link-dead-code" cargo test --verbose --color always --features hashbrown


I dunno if we want the last test we run in "matrix.coverage" (ie the binaries which we generate coverage reports from) to be hashbrown ones - maybe move this up above (or as the first command in) "Test on Rust ${{ matrix.toolchain }} with net-tokio and full code-linking for coverage generation"?

SGTM, wasn't sure, so just added it after the other tests.

hrm, seems like this causes build failure:

error[E0658]: use of unstable library feature 'ptr_cast' --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/hashbrown-0.11.2/src/raw/mod.rs:504:57 | 504 | NonNull::new_unchecked(self.table.ctrl.as_ptr().cast()) | ^^^^ | = note: for more information, see https://github.com/rust-lang/rust/issues/60602

Might only make sense to use hashbrown in later versions of Rust, per rust-lang/rust#60602

MSRV of hashbrown 0.11.2 is rust 1.49

Right, it needs to only run on non-1.36 versions of rust. Maybe add this:

- name: Test no_std builds Rust ${{ matrix.toolchain }} if: "matrix.build-net-tokio" run: cargo test --verbose --color always --features hashbrown

MSRV of hashbrown 0.11.2 is rust 1.49

Oh, I missed this comment, I guess we need the if there to just be something new that matches on stable instaed.

Try:

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 231db705..2f14a4c2 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -14,18 +14,27 @@ jobs: # 1.41.0 is Debian stable 1.41.0, # 1.45.2 is MSRV for lightning-net-tokio, lightning-block-sync, and coverage generation - 1.45.2] + 1.45.2, + # 1.49 is MSRV for no_std + 1.49.0] include: - toolchain: stable build-net-tokio: true + build-no-std: true - toolchain: stable platform: macos-latest build-net-tokio: true + build-no-std: true - toolchain: stable platform: windows-latest build-net-tokio: true + build-no-std: true - toolchain: beta build-net-tokio: true + build-no-std: true + - toolchain: 1.45.2 + build-net-tokio: true + build-no-std: true - toolchain: 1.45.2 build-net-tokio: true coverage: true @@ -76,6 +85,9 @@ jobs: cd ../lightning RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always --features hashbrown cd .. + - name: Test no_std builds Rust ${{ matrix.toolchain }} + if: "matrix.build-no-std" + run: cargo test --verbose --color always --features hashbrown - name: Test on Rust ${{ matrix.toolchain }} with net-tokio if: "matrix.build-net-tokio && !matrix.coverage" run: cargo test --verbose --color always

I added a check build-no-std, and a 1.49.0 to the toolchain. It took a little experimentation, but I think I understand the CI build scripts better now.

The fuzz build seems to be failing for other reasons...

TheBlueMatt

Looks good, only diff since devrandom's ack at d9ca4ea are CI and comment addressing, fuzz test failure is due to a libsecp bump that exists on current HEAD anyway, so shouldn't hold this up.

GeneFerneau commented Jun 10, 2021

View reviewed changes

GeneFerneau force-pushed the hash branch 2 times, most recently from 5c10136 to d9ca4ea Compare June 10, 2021 03:24

devrandom approved these changes Jun 10, 2021

View reviewed changes

lightning/src/routing/router.rs Show resolved Hide resolved

lightning/src/lib.rs Show resolved Hide resolved

TheBlueMatt reviewed Jun 10, 2021

View reviewed changes

lightning/Cargo.toml Outdated Show resolved Hide resolved

TheBlueMatt reviewed Jun 10, 2021

View reviewed changes

GeneFerneau force-pushed the hash branch from d9ca4ea to 9b938f7 Compare June 11, 2021 01:09

TheBlueMatt approved these changes Jun 11, 2021

View reviewed changes

GeneFerneau force-pushed the hash branch 4 times, most recently from 75b8a14 to 9bac852 Compare June 11, 2021 21:18

TheBlueMatt approved these changes Jun 12, 2021

View reviewed changes

GeneFerneau force-pushed the hash branch 5 times, most recently from 0b09ee8 to e76a37f Compare June 18, 2021 21:43

Use hashbrown replacements for std equivalents

da7a851

GeneFerneau force-pushed the hash branch from e76a37f to da7a851 Compare June 18, 2021 21:55

TheBlueMatt approved these changes Jun 20, 2021

View reviewed changes

TheBlueMatt merged commit 94528f0 into lightningdevkit:main Jun 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use hashbrown replacements for std equivalents #947

Use hashbrown replacements for std equivalents #947

GeneFerneau commented Jun 10, 2021

GeneFerneau Jun 10, 2021

TheBlueMatt commented Jun 10, 2021

codecov bot commented Jun 10, 2021 •

edited

Loading

devrandom left a comment

TheBlueMatt Jun 10, 2021 •

edited

Loading

GeneFerneau Jun 10, 2021

TheBlueMatt Jun 10, 2021

GeneFerneau Jun 10, 2021

TheBlueMatt Jun 11, 2021

GeneFerneau Jun 11, 2021

TheBlueMatt Jun 11, 2021

TheBlueMatt Jun 11, 2021

GeneFerneau Jun 11, 2021

TheBlueMatt Jun 11, 2021

GeneFerneau Jun 11, 2021

TheBlueMatt Jun 12, 2021

TheBlueMatt Jun 12, 2021

GeneFerneau Jun 18, 2021

GeneFerneau Jun 18, 2021

GeneFerneau Jun 18, 2021 •

edited

Loading

TheBlueMatt Jun 18, 2021

TheBlueMatt Jun 18, 2021

TheBlueMatt Jun 18, 2021

GeneFerneau Jun 18, 2021

TheBlueMatt left a comment

Use hashbrown replacements for std equivalents #947

Use hashbrown replacements for std equivalents #947

Conversation

GeneFerneau commented Jun 10, 2021

Choose a reason for hiding this comment

TheBlueMatt commented Jun 10, 2021

codecov bot commented Jun 10, 2021 • edited Loading

Codecov Report

devrandom left a comment

Choose a reason for hiding this comment

TheBlueMatt Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeneFerneau Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2021 •

edited

Loading

TheBlueMatt Jun 10, 2021 •

edited

Loading

GeneFerneau Jun 18, 2021 •

edited

Loading