Simplify and fix AtomicCounter #3302

TheBlueMatt · 2024-09-08T20:19:46Z

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 #3000

codecov · 2024-09-08T20:26:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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

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

lightning/src/ln/monitor_tests.rs

lightning/src/util/atomic_counter.rs

tnull · 2024-09-09T08:02:19Z

lightning/src/util/atomic_counter.rs

+			#[cfg(target_has_atomic = "64")]
+			counter: AtomicU64::new(0),
+			#[cfg(not(target_has_atomic = "64"))]
+			counter: Mutex::new(0),
 		}
 	}
 	pub(crate) fn get_increment(&self) -> u64 {


nit: Preexisting, this naming seems a bit confusing as we're actually not returning the incremented counter. Maybe the field should be called next_counter and the method just next()?

tnull

LGTM, I think. Feel free to squash the fixup.

TheBlueMatt · 2024-09-09T14:40:32Z

Squashed.

tnull · 2024-09-11T07:42:32Z

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 · 2024-09-11T14:09:07Z

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;
$

tnull

LGTM, happy to land it if CI passes.

`InMemorySigner` has various private keys in it which makes `Debug` either useless or dangerous (because most keys won't log anything, but if they did we'd risk logging private key material).

`blinded_path_with_custom_tlv` indirectly relied on route CLTV randomization when sending because nodes were at substantially different block heights after setup. Instead we make sure all nodes are at the same height which makes the test more robust.

TheBlueMatt · 2024-09-11T20:02:10Z

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 · 2024-09-12T07:22:08Z

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 · 2024-09-12T13:53:09Z

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;
 		}
$

`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 lightningdevkit#3000

Its a counter, `next` is super clear, `get_increment` is a bit less so.

tnull

LGTM, landing this as pretty trivial.

Kixunil · 2024-09-12T18:57:24Z

lightning/src/util/atomic_counter.rs

+	#[cfg(target_has_atomic = "64")]
+	counter: AtomicU64,
+	#[cfg(not(target_has_atomic = "64"))]
+	counter: Mutex<u64>,


If the intention is to produce some unique values that maybe don't need to be sequential you can still have a sensible lock-free implementation. Roughly like this:

let mut low = self.low.load(Relaxed); let mut high = self.high.load(Relaxed); loop { let new_low = if low == u32::MAX { // don't use fetch_add to avoid incrementing high by more than 1 if let Err(new) = self.high.compare_exchange(high, high + 1, Relaxed, Relaxed) { high = new; } 0 } else { low + 1 } // FTR this cannot be weak match self.low.compare_exchange(low, new_low, Relaxed, Relaxed) { Ok(_) => break, Err(new) => low = new, } } u64::from(high) << 32 | u64::from(low)

This assumes that a thread doesn't get scheduled-out after incrementing high for so long that other thread(s) manage to increment the counter by 2^32, which I think is a reasonable assumption. There's still a chance that high gets bumped by more than one though if a thread managed to bump it and before it updates low another thread reads both of them. This is quite unfrequent and could be dealt with by sacrificing one bit of high which gets set first and then reset at the end.

Kixunil · 2024-09-12T18:58:16Z

lightning/src/util/atomic_counter.rs

-		};
-		(high << 32) | low
+		#[cfg(target_has_atomic = "64")] {
+			self.counter.fetch_add(1, Ordering::AcqRel)


You have AcqRel however to my understanding this is not a synchronization primitive so Relaxed is appropriate.

TheBlueMatt mentioned this pull request Sep 8, 2024

Add a PaymentId for inbound payments #3303

Merged

tnull self-requested a review September 9, 2024 07:53

tnull reviewed Sep 9, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-09-atomic-cleanups branch from 3b9487b to 7b1f07b Compare September 9, 2024 14:40

TheBlueMatt mentioned this pull request Sep 9, 2024

test_restored_packages_retry is brittle #2827

Open

TheBlueMatt force-pushed the 2024-09-atomic-cleanups branch from 7b1f07b to 38ecd35 Compare September 11, 2024 14:08

tnull previously approved these changes Sep 11, 2024

View reviewed changes

TheBlueMatt added 2 commits September 11, 2024 20:01

Drop Debug on InMemorySigner (and EntropySource)

758747a

`InMemorySigner` has various private keys in it which makes `Debug` either useless or dangerous (because most keys won't log anything, but if they did we'd risk logging private key material).

TheBlueMatt dismissed tnull’s stale review via dccdd3c September 11, 2024 20:01

TheBlueMatt force-pushed the 2024-09-atomic-cleanups branch from 38ecd35 to dccdd3c Compare September 11, 2024 20:01

TheBlueMatt force-pushed the 2024-09-atomic-cleanups branch from dccdd3c to 68d4c96 Compare September 12, 2024 13:52

TheBlueMatt added 2 commits September 12, 2024 14:32

Rename AtomicCounter::get_increment to simply next

1c2bd09

Its a counter, `next` is super clear, `get_increment` is a bit less so.

TheBlueMatt force-pushed the 2024-09-atomic-cleanups branch from 68d4c96 to 1c2bd09 Compare September 12, 2024 14:32

tnull approved these changes Sep 12, 2024

View reviewed changes

tnull merged commit a75fdab into lightningdevkit:main Sep 12, 2024
16 of 19 checks passed

Kixunil reviewed Sep 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and fix AtomicCounter #3302

Simplify and fix AtomicCounter #3302

TheBlueMatt commented Sep 8, 2024

codecov bot commented Sep 8, 2024 •

edited

Loading

tnull Sep 9, 2024

tnull left a comment

TheBlueMatt commented Sep 9, 2024

tnull commented Sep 11, 2024

TheBlueMatt commented Sep 11, 2024

tnull left a comment

TheBlueMatt commented Sep 11, 2024

tnull commented Sep 12, 2024

TheBlueMatt commented Sep 12, 2024 •

edited

Loading

tnull left a comment

Kixunil Sep 12, 2024

Kixunil Sep 12, 2024

Simplify and fix AtomicCounter #3302

Simplify and fix AtomicCounter #3302

Conversation

TheBlueMatt commented Sep 8, 2024

codecov bot commented Sep 8, 2024 • edited Loading

Codecov Report

tnull Sep 9, 2024

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 9, 2024

tnull commented Sep 11, 2024

TheBlueMatt commented Sep 11, 2024

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 11, 2024

tnull commented Sep 12, 2024

TheBlueMatt commented Sep 12, 2024 • edited Loading

tnull left a comment

Choose a reason for hiding this comment

Kixunil Sep 12, 2024

Choose a reason for hiding this comment

Kixunil Sep 12, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 8, 2024 •

edited

Loading

TheBlueMatt commented Sep 12, 2024 •

edited

Loading