lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.16k stars 367 forks source link

Simplify and fix AtomicCounter #3302

Closed TheBlueMatt closed 2 months ago

TheBlueMatt commented 2 months ago

AtomicCounter was slightly race-y on 32-bit platforms because it increments the high AtomicUsize independently from the low AtomicUsize, leading to a potential race where another thread could observe the low increment but not the high increment and see a value of 0 twice.

This isn't a big deal because (a) most platforms are 64-bit these days, (b) 32-bit platforms aren't super likely to have their counter overflow 32 bits anyway, and (c) the two writes are back-to-back so having another thread read during that window is very unlikely.

However, we can also optimize the counter somewhat by using the target_has_atomic = "64" cfg flag, which we do here, allowing us to use AtomicU64 even on 32-bit platforms where 64-bit atomics are available.

This changes some test behavior slightly, which requires adaptation.

Fixes https://github.com/lightningdevkit/rust-lightning/issues/3000

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.57%. Comparing base (d35239c) to head (1c2bd09). Report is 33 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3302 +/- ## ========================================== + Coverage 89.85% 90.57% +0.72% ========================================== Files 126 126 Lines 104145 109040 +4895 Branches 104145 109040 +4895 ========================================== + Hits 93577 98761 +5184 + Misses 7894 7624 -270 + Partials 2674 2655 -19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

TheBlueMatt commented 2 months ago

Squashed.

tnull commented 2 months ago

Squashed.

It seems you're missing some imports here, CI is sad:

error[E0412]: cannot find type `Mutex` in this scope
  --> /home/runner/work/rust-lightning/rust-lightning/lightning/src/util/atomic_counter.rs:13:11
   |
13 |     counter: Mutex<u64>,
   |              ^^^^^ not found in this scope
   |
help: consider importing this struct through its public re-export
   |
5  + use crate::sync::Mutex;
   |

etc

TheBlueMatt commented 2 months ago

Gah

$ git diff-tree -U1 7b1f07bd3 38ecd35c7
diff --git a/lightning/src/util/atomic_counter.rs b/lightning/src/util/atomic_counter.rs
index d00c7341b..6dbe54e64 100644
--- a/lightning/src/util/atomic_counter.rs
+++ b/lightning/src/util/atomic_counter.rs
@@ -5,3 +5,3 @@ use core::sync::atomic::{AtomicU64, Ordering};
 #[cfg(not(target_has_atomic = "64"))]
-use crate::prelude::*;
+use crate::sync::Mutex;
$ 
TheBlueMatt commented 2 months ago

FFS

$ git diff-tree -U1 38ecd35c7 dccdd3c15
diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs
index 41354c695..4b8a9e025 100644
--- a/lightning/src/sign/mod.rs
+++ b/lightning/src/sign/mod.rs
@@ -1029,3 +1029,2 @@ pub trait ChangeDestinationSource {
 /// a secure external signer.
-#[derive(Debug)]
 pub struct InMemorySigner {
@@ -2483,3 +2482,2 @@ impl PhantomKeysManager {
 /// An implementation of [`EntropySource`] using ChaCha20.
-#[derive(Debug)]
 pub struct RandomBytes {
diff --git a/lightning/src/util/atomic_counter.rs b/lightning/src/util/atomic_counter.rs
index 6dbe54e64..3627ccf8a 100644
--- a/lightning/src/util/atomic_counter.rs
+++ b/lightning/src/util/atomic_counter.rs
@@ -7,3 +7,2 @@ use crate::sync::Mutex;

-#[derive(Debug)]
 pub(crate) struct AtomicCounter {
$ 
tnull commented 2 months ago

FFS

Nope:

error[E0596]: cannot borrow `mtx` as mutable, as it is not declared as mutable
  --> /home/runner/work/rust-lightning/rust-lightning/lightning/src/util/atomic_counter.rs:30:5
   |
30 |             *mtx += 1;
   |              ^^^ cannot borrow as mutable
   |
help: consider changing this to be mutable
   |
29 |             let mut mtx = self.counter.lock().unwrap();
   |                 +++

For more information about this error, try `rustc --explain E0596`.
warning: `lightning` (lib) generated 1 warning
error: could not compile `lightning` (lib) due to 1 previous error; 1 warning emitted
Error: Process completed with exit code 101.
TheBlueMatt commented 2 months ago

Grr, guess I didn't have the arm compiler installed so running CI locally didn't test it...

$ git diff-tree -U2 dccdd3c15 68d4c9673
diff --git a/lightning/src/util/atomic_counter.rs b/lightning/src/util/atomic_counter.rs
index 3627ccf8a..b1fd7323f 100644
--- a/lightning/src/util/atomic_counter.rs
+++ b/lightning/src/util/atomic_counter.rs
@@ -38,5 +38,5 @@ impl AtomicCounter {
        }
        #[cfg(not(target_has_atomic = "64"))] {
-           let mtx = self.counter.lock().unwrap();
+           let mut mtx = self.counter.lock().unwrap();
            *mtx  = count;
        }
$