feat(txpool|p2p): use seqlock instead of small copy-able RwLocks

[rymnc]

Linked Issues/PRs

• none

Description

We utilize SeqLock instead of an RwLock for data that satisfies the following conditions -

1. Occasional writes
2. Frequent reads

The write method no longer has a hot loop, and therefore writes aren't blocked by reads. we don't use an internal locking mechanism, but rather a sequence counter to check if data was changed before/during/after a read. This may result in a few iterations while reading, but it is minimal.

We should have a benchmark for txpool to see the performance difference.

It is important to note that this implementation assumes that there will only be 1 writer thread.

Checklist

• Breaking changes are clearly marked as such in the PR description and changelog
• New behavior is reflected in tests
• The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

• I have reviewed the code myself
• I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

• Rust SDK
• Sway compiler
• Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
• Someone else?

[acerone85]

What happens if two writers try to update the value concurrently?

[rymnc]

well they shouldn't
this is designed for single writer only
there's a lot of foot guns with this implementation, and makes it highly specific for one use case -

1. single writer
2. small data structure
3. heavy reads

I can add as a warning comment if you'd like

I want to avoid write locks because this assumes only a single writer exists

[rymnc]

lmk if this comment works: 118c37a

[acerone85]

thanks, the comment looks good :)

[MitchTurner]

Can't we guarantee that only a single writer exists? Like, on construction we just generate a SeqLockWriter and from that we can generate any number of SeqLockReaders, but it's SeqLockWriter isn't cloneable?

Essentially like a single producer, multiple consumer channel.

Internally, it's the same, but we expose different mut apis for the SeqLockWriter.

[rymnc]

addressed in 720d486

[MitchTurner]

We could create a trait for this T to guarantee that it's below 64 bytes and provide some default impls for the types we expect to see.

/// Type is equal to or smaller than 64 bytes
pub trait Small;

[netrome]

(chiming in) perhaps Small should imply Copy in this case: pub trait Small: Copy;

[rymnc]

i don't quite like the idea of creating default impls here ~ we won't be able to implement the trait for types outside the crate either due to the orphan rule.

I can enforce the type size at runtime if you'd like.

[netrome]

Overall looks good to me. I'd propose making the SeqLock struct private, potentially including the T: Clone bound directly on the struct (though I'm not sure of this). I'm okay with the current implementation as well.

[netrome]

Is it unsafe or just inefficient if the data isn't Copy? My hunch is that it's the latter.

I'm also thinking if we should actually add the trait bound on the struct itself. In general we should avoid trait bounds on structs, but the SeqLock seems to be meaningless if the data isn't Copy. Would there ever be a use case for it with non-Copy data?

[rymnc]

it is unsafe if the data is not copy because of torn reads

[Dentosal]

If we require that T: Copy here, compiler will enforce that for us.

[rymnc]

addressed in 6d626b5

[Dentosal]

If this is required for safety, then the constructor must be marked as unsafe fn since it's up to the caller to guarantee it.

[rymnc]

addressed in 6d626b5

[Dentosal]

Is there a safety reason for the 64 bytes limit? Is it < or <=?

[rymnc]

<= fixed in 51d079c

yes there is a safety reason, i have added the comments necessary to describe it

[Dentosal]

The docs should mention that Clone bound is missing to prevent creating multiple writers.

[rymnc]

addressed in 6d626b5

[Dentosal]

If F panics, the lock is in invalid state. Since locks are shared between threads, that seems to be an actual issue.

[rymnc]

addressed in 6d626b5

[Dentosal]

Needs // Safety comment to explain why the `unsafe´ is acceptable.

[rymnc]

addressed in 72e09f9

[Dentosal]

After some further reading, I believe this construct is unsound. The compiler is allowed to reorder the non-atomic write to the UnsafeCell to occur before the first fetch_add, or after the second one. This in turn leads to aliased mut and/or a data race, which is UB.

My first idea on how to fix this would be to use another AtomicU64 to hold the data, and convert the values using std::mem::transmute_copy or similar perform conversion. Alternatively volatile ptr operations could be used, but that might have soundness holes as well.

I've reviewed the seqlock crate which looks decent for this, but it doesn't require single writer, which means it's lacking some optimizations that we could have here.

[rymnc]

shouldn't the fetch_add prevent reordering of UnsafeCell writes?

[rymnc]

curious if using a fence would fix this

[Dentosal]

I believe that the following access pattern is valid (note the first fetch_add having AcqRel ordering):

    lock.sequence.fetch_add(1, Ordering::AcqRel);
    std::sync::atomic::fence(Ordering::Acquire);
    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| unsafe {
        let data = &mut *lock.data.get();
        f(data);
    }));
    std::sync::atomic::fence(Ordering::Release);
    lock.sequence.fetch_add(1, Ordering::Release);
    if let Err(e) = result {
        std::panic::resume_unwind(e);
    }

[rymnc]

addressed in 51d079c

if your main concern is with the compiler reordering then we can use compiler_fence instead but that doesn't prevent the hardware from doing so, so I don't mind using fence here

[Dentosal]

We should probably call std::thread::yield_now before continue. See https://github.com/Amanieu/seqlock/blob/b72653fd9c38141da7e588a97a1b654239f5d4df/src/lib.rs#L126

[rymnc]

addressed in 51d079c

[Dentosal]

Missing // Safety comment. The unsoundness issue from the write section also applies here.

[rymnc]

addressed in 51d079c

[Dentosal]

This is already ensured by the type, so having it inside !!WARNING!! section seems excessive

[rymnc]

addressed in 51d079c