Implement full-duplex secret connection #938

thanethomson · 2021-07-23T14:34:28Z

Work towards #814

Something like this is necessary for the MConnection to be able to safely read from and write to the underlying stream from multiple threads (e.g. one thread for reading, one thread for writing).

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Added entry in .changelog/

Signed-off-by: Thane Thomson <connect@thanethomson.com>

This adds a `TcpStream`-based test for parallelizing operations on a `SecretConnection`. I used `TcpStream` instead of the buffered reader in the other tests because it wasn't feasible to implement the `TryClone` trait for that buffered pipe implementation. Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

codecov-commenter · 2021-07-23T17:52:17Z

Codecov Report

Merging #938 (ce9290d) into master (cedf8de) will increase coverage by 0.0%.
The diff coverage is 85.2%.

❗ Current head ce9290d differs from pull request most recent head 2d5ef8b. Consider uploading reports for the commit 2d5ef8b to get more accurate results

@@           Coverage Diff           @@
##           master    #938    +/-   ##
=======================================
  Coverage    70.6%   70.7%            
=======================================
  Files         203     205     +2     
  Lines       16312   16432   +120     
=======================================
+ Hits        11524   11624   +100     
- Misses       4788    4808    +20

Impacted Files	Coverage Δ
p2p/src/secret_connection.rs	`85.0% <78.6%> (+0.2%)`	⬆️
std-ext/src/lib.rs	`100.0% <100.0%> (ø)`
std-ext/src/try_clone.rs	`100.0% <100.0%> (ø)`
test/src/test/unit/p2p/secret_connection.rs	`99.3% <100.0%> (+0.4%)`	⬆️
tendermint/src/block/commit_sig.rs	`77.2% <0.0%> (-2.3%)`	⬇️
tendermint/src/timeout.rs	`83.0% <0.0%> (-1.9%)`	⬇️
tendermint/src/config.rs	`64.2% <0.0%> (-1.7%)`	⬇️
light-client/src/types.rs	`30.6% <0.0%> (-0.3%)`	⬇️
tendermint/src/node.rs	`65.7% <0.0%> (-0.3%)`	⬇️
tools/proto-compiler/src/constants.rs	`6.2% <0.0%> (ø)`
... and 2 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 cedf8de...2d5ef8b. Read the comment docs.

p2p/src/secret_connection.rs

soareschen · 2021-07-26T08:59:25Z

p2p/src/transport.rs

+    fn try_clone(&self) -> Result<Self, Self::Error> {
+        // Uses the TcpStream struct's method. See
+        // https://doc.rust-lang.org/stable/book/ch19-03-advanced-traits.html#fully-qualified-syntax-for-disambiguation-calling-methods-with-the-same-name
+        self.try_clone()


Would it be simpler if we just use TcpStream::try_clone(self) here and not having to explain in the comment?

It wasn't obvious to me whether the TcpStream's try_clone method was going to be called here or the TryClone trait's try_clone, so I thought I'd leave a comment here for posterity to explain it.

Oh, also, I tried explicitly calling TcpStream::try_clone(self) here and clippy complains that the better approach is to use Self::try_clone(self), which, to me, isn't nearly as clear as TcpStream::try_clone(self).

soareschen · 2021-07-26T09:01:25Z

p2p/src/secret_connection.rs

@@ -452,6 +496,23 @@ where
    }
 }

+impl<IoHandler: TryClone> TryClone for SecretConnection<IoHandler> {


What is the advantage of using this trait as compared to holding an Arc<IoHandler> and Clone?

See #938 (comment)

soareschen · 2021-07-26T09:06:07Z

p2p/src/secret_connection.rs

            if let Err(err) = res {
                return Err(io::Error::new(io::ErrorKind::Other, err.to_string()));
            }
-            self.send_nonce.increment();
+            send_state.send_nonce.increment();


Should we have higher level abstraction to guarantee that the nonce is always incremented after being acquired? Or should it never be incremented at all if there is error? Would the state be inconsistent if the line above or below returns error prematurely?

@tony-iqlusion, what's your take on this?

Preventing nonce reuse is the most important thing as it fails catastrophically with ChaCha20Poly1305, but also if it's ever incremented without being used it will be out-of-sync with the peer and abort the connection that way.

So it's best to have a sort of simple state machine that aborts the whole connection on errors, and if that can wipe the nonce somehow that's best, but so long as you can prevent repeat nonce reuse that's probably the most important thing.

p2p/src/secret_connection.rs

soareschen · 2021-07-26T09:09:11Z

p2p/src/secret_connection.rs

+        let mut recv_state = self
+            .recv_state
+            .lock()
+            .expect("failed to obtain lock on secret connection recv state");


Should we return error result here instead of panicking? Same for the other uses of .expect().

How would we (and should we be able to) recover from such an error?

My understanding is that, when Mutex::lock fails, it's due to a panic while the lock is held by another thread.

Sounds reasonable. Perhaps we can add a comment here that we are panicking because a poisoned mutex is unrecoverable. This is mainly to distinguish it from the other uses of .expect in the same file, which IMO should be refactored to return Result::Err instead.

Fortunately in the new iteration there's no need for mutexes any more 😁

thanethomson · 2021-07-26T12:49:10Z

I've converted this back to a draft PR while I rework things a bit as per Tony's suggestions.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

…nd receiving halves Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

tony-iqlusion

Looks good now!

xla

Very sensible, mostly left some clarifying questions. Are the concerns addressed wrt nonce handling in #938 (comment)?

p2p/src/transport.rs

test/src/test/unit/p2p/secret_connection.rs

xla · 2021-07-27T13:43:25Z

p2p/src/secret_connection.rs

+    debug_assert!(!chunk.is_empty(), "chunk is empty");
+    debug_assert!(


Do we only want to guard against these conditions in debug builds?

Good question. @tony-iqlusion?

p2p/src/secret_connection.rs

thanethomson · 2021-07-27T14:29:14Z

Are the concerns addressed wrt nonce handling in #938 (comment)?

I'm not at all familiar with the low-level details of ChaCha20Poly1305.

@tony-iqlusion, does this mean that, if there's any failure to read from/write to a SecretConnection, the whole connection needs to be dropped and re-established to be able to continue communicating? If so that's something I should probably add to the documentation.

This introduces the internal encryption assertions at runtime regardless of build type. This may introduce a small performance hit, but it's probably worth it to ensure correctness. Effectively this is keeping an eye on the code in the `encrypt_and_write` fn to ensure its correctness. Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

…m constraints Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson · 2021-07-27T21:56:39Z

The latest commits build incrementally on previous ones, so review shouldn't be too complex.

One question though for those who've done/seen this sort of performance benchmarking in Rust before: what's the potential performance hit of loading an AtomicBool's value before each time we read from/write to the underlying TCP stream?

tony-iqlusion · 2021-07-27T22:19:13Z

what's the potential performance hit of loading an AtomicBool's value before each time we read from/write to the underlying TCP stream?

Vanishingly small compared to the system call overhead of an I/O operation. On x86(-64) in particular, the architecture provides a strongly consistent memory model, which means AtomicBool is effectively zero overhead versus an unsynchronized load/store regardless of what consistency model you choose.

Things are a bit more complicated on e.g. ARM, where it will require flushing (potentially multicore) CPU pipelines, but again, minuscule compare to the overhead of a system call.

thanethomson · 2021-07-27T22:36:20Z

Sounds good, thanks for the insight @tony-iqlusion! 🙏

p2p/src/secret_connection.rs

xla

With #905 (comment) in mind should the internals and the handles acquired from a split be named Reader and Writer respectively? Esp. as they implement Read and Write and receiver and sender can be reserved for abstractions which offer full messages as they input/output.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson · 2021-07-28T18:58:11Z

With #905 (comment) in mind should the internals and the handles acquired from a split be named Reader and Writer respectively? Esp. as they implement Read and Write and receiver and sender can be reserved for abstractions which offer full messages as they input/output.

I've been trying to figure out which set of idioms (either "send/receive" or "read/write") should apply where, and it seems to me like "read/write" seems more general than "send/receive" (e.g. the latter doesn't make too much sense when interacting with the filesystem).

Networked entities seem to send/receive more than read/write. This is probably due to the fact that low-level socket functions like send employ "send/receive" semantics on networking connections. As such, it probably makes more sense to keep the SecretConnection's "send/receive" semantics. It's already pretty pervasive throughout the SecretConnection-related code.

But either way, as long as it's consistent, I'd be fine with it. My issue in #905 (comment) was that it was inconsistent.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

xla

🏌 😜 🏷 🏉

* Implement full-duplex secret connection (#938) * Implement thread-safe cloning of a secret connection Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand documentation for SecretConnection on threading considerations Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract peer construction into its own method Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add test for cloned SecretConnection This adds a `TcpStream`-based test for parallelizing operations on a `SecretConnection`. I used `TcpStream` instead of the buffered reader in the other tests because it wasn't feasible to implement the `TryClone` trait for that buffered pipe implementation. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add more messages to test Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add .changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Restore half-duplex operations Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract encrypt/decrypt fns as independent methods Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove unnecessary trait bounds Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract send/receive state Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract read/write functionality as standalone methods Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add logic to facilitate splitting SecretConnection into its sending and receiving halves Signed-off-by: Thane Thomson <connect@thanethomson.com> * Restore split SecretConnection test using new semantics Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update docs for `SecretConnection` Signed-off-by: Thane Thomson <connect@thanethomson.com> * Condense error reporting Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract TryClone trait into its own crate As per the discussion at #938 (comment), this extracts the `TryClone` trait into a new crate called `tendermint-std-ext` in the `std-ext` directory. This new crate is intended to contain any code that we need that extends the Rust standard library. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Reorder imports Signed-off-by: Thane Thomson <connect@thanethomson.com> * Assert validation regardless of debug build This introduces the internal encryption assertions at runtime regardless of build type. This may introduce a small performance hit, but it's probably worth it to ensure correctness. Effectively this is keeping an eye on the code in the `encrypt_and_write` fn to ensure its correctness. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove remote_pubkey optionality from sender/receiver halves Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update SecretConnection docs with comment content Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add docs on SecretConnection failures and connection integrity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Synchronize sending/receiving failures to comply with crypto algorithm constraints Signed-off-by: Thane Thomson <connect@thanethomson.com> * Rename try_split method to split for SecretConnection Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove redundant field name prefixes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix broken link in docs Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix recent clippy errors on `master` (#941) * Fix needless borrows in codebase Signed-off-by: Thane Thomson <connect@thanethomson.com> * Ignore needless collect warning (we do actually seem to need it) Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove trailing semicolon in macro to fix docs compiling Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Use flex-error for tendermint * Use flex-error for p2p * Use flex-error for light-client * Use flex-error for light_client::predicates * Fix lint * Use flex-error for builder and io errors * Use flex-error for rpc errors * Use flex-error for protobuf errors * Use flex-error for abci * Fix test_bisection_no_witness_left * Fix build errors in all-features * Fix failing tests * Fix more failures * Fix tungstenite error under wasm target * Fix incoming_fixtures test * Fix conflict * Update flex-error to v0.4.0 * set std feature in flex-error instead of individual crates * Add flex-error patch to tools/Cargo.toml * Use published version of flex-error v0.4.1 * Enable flex-error/eyre_tracer feature by default * Add .changelog entry (#940) Signed-off-by: Thane Thomson <connect@thanethomson.com> * flex-error: resolve conflicts with `master` (#945) * Implement full-duplex secret connection (#938) * Implement thread-safe cloning of a secret connection Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand documentation for SecretConnection on threading considerations Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract peer construction into its own method Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add test for cloned SecretConnection This adds a `TcpStream`-based test for parallelizing operations on a `SecretConnection`. I used `TcpStream` instead of the buffered reader in the other tests because it wasn't feasible to implement the `TryClone` trait for that buffered pipe implementation. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add more messages to test Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add .changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Restore half-duplex operations Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract encrypt/decrypt fns as independent methods Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove unnecessary trait bounds Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract send/receive state Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract read/write functionality as standalone methods Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add logic to facilitate splitting SecretConnection into its sending and receiving halves Signed-off-by: Thane Thomson <connect@thanethomson.com> * Restore split SecretConnection test using new semantics Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update docs for `SecretConnection` Signed-off-by: Thane Thomson <connect@thanethomson.com> * Condense error reporting Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract TryClone trait into its own crate As per the discussion at #938 (comment), this extracts the `TryClone` trait into a new crate called `tendermint-std-ext` in the `std-ext` directory. This new crate is intended to contain any code that we need that extends the Rust standard library. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Reorder imports Signed-off-by: Thane Thomson <connect@thanethomson.com> * Assert validation regardless of debug build This introduces the internal encryption assertions at runtime regardless of build type. This may introduce a small performance hit, but it's probably worth it to ensure correctness. Effectively this is keeping an eye on the code in the `encrypt_and_write` fn to ensure its correctness. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove remote_pubkey optionality from sender/receiver halves Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update SecretConnection docs with comment content Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add docs on SecretConnection failures and connection integrity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Synchronize sending/receiving failures to comply with crypto algorithm constraints Signed-off-by: Thane Thomson <connect@thanethomson.com> * Rename try_split method to split for SecretConnection Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove redundant field name prefixes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix broken link in docs Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix recent clippy errors on `master` (#941) * Fix needless borrows in codebase Signed-off-by: Thane Thomson <connect@thanethomson.com> * Ignore needless collect warning (we do actually seem to need it) Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove trailing semicolon in macro to fix docs compiling Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove unnecessary macros Signed-off-by: Thane Thomson <connect@thanethomson.com> * Correct error messages Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Thane Thomson <thane@informal.systems> Co-authored-by: Thane Thomson <connect@thanethomson.com>

* Implement thread-safe cloning of a secret connection Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand documentation for SecretConnection on threading considerations Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract peer construction into its own method Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add test for cloned SecretConnection This adds a `TcpStream`-based test for parallelizing operations on a `SecretConnection`. I used `TcpStream` instead of the buffered reader in the other tests because it wasn't feasible to implement the `TryClone` trait for that buffered pipe implementation. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add more messages to test Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add .changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Restore half-duplex operations Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract encrypt/decrypt fns as independent methods Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove unnecessary trait bounds Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract send/receive state Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract read/write functionality as standalone methods Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add logic to facilitate splitting SecretConnection into its sending and receiving halves Signed-off-by: Thane Thomson <connect@thanethomson.com> * Restore split SecretConnection test using new semantics Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update docs for `SecretConnection` Signed-off-by: Thane Thomson <connect@thanethomson.com> * Condense error reporting Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract TryClone trait into its own crate As per the discussion at informalsystems/tendermint-rs#938 (comment), this extracts the `TryClone` trait into a new crate called `tendermint-std-ext` in the `std-ext` directory. This new crate is intended to contain any code that we need that extends the Rust standard library. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Reorder imports Signed-off-by: Thane Thomson <connect@thanethomson.com> * Assert validation regardless of debug build This introduces the internal encryption assertions at runtime regardless of build type. This may introduce a small performance hit, but it's probably worth it to ensure correctness. Effectively this is keeping an eye on the code in the `encrypt_and_write` fn to ensure its correctness. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove remote_pubkey optionality from sender/receiver halves Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update SecretConnection docs with comment content Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add docs on SecretConnection failures and connection integrity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Synchronize sending/receiving failures to comply with crypto algorithm constraints Signed-off-by: Thane Thomson <connect@thanethomson.com> * Rename try_split method to split for SecretConnection Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove redundant field name prefixes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix broken link in docs Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Use flex-error for tendermint * Use flex-error for p2p * Use flex-error for light-client * Use flex-error for light_client::predicates * Fix lint * Use flex-error for builder and io errors * Use flex-error for rpc errors * Use flex-error for protobuf errors * Use flex-error for abci * Fix test_bisection_no_witness_left * Fix build errors in all-features * Fix failing tests * Fix more failures * Fix tungstenite error under wasm target * Fix incoming_fixtures test * Fix conflict * Update flex-error to v0.4.0 * set std feature in flex-error instead of individual crates * Add flex-error patch to tools/Cargo.toml * Use published version of flex-error v0.4.1 * Enable flex-error/eyre_tracer feature by default * Add .changelog entry (#940) Signed-off-by: Thane Thomson <connect@thanethomson.com> * flex-error: resolve conflicts with `master` (#945) * Implement full-duplex secret connection (#938) * Implement thread-safe cloning of a secret connection Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand documentation for SecretConnection on threading considerations Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract peer construction into its own method Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add test for cloned SecretConnection This adds a `TcpStream`-based test for parallelizing operations on a `SecretConnection`. I used `TcpStream` instead of the buffered reader in the other tests because it wasn't feasible to implement the `TryClone` trait for that buffered pipe implementation. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add more messages to test Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand comment for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add .changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Restore half-duplex operations Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract encrypt/decrypt fns as independent methods Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove unnecessary trait bounds Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract send/receive state Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract read/write functionality as standalone methods Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add logic to facilitate splitting SecretConnection into its sending and receiving halves Signed-off-by: Thane Thomson <connect@thanethomson.com> * Restore split SecretConnection test using new semantics Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update docs for `SecretConnection` Signed-off-by: Thane Thomson <connect@thanethomson.com> * Condense error reporting Signed-off-by: Thane Thomson <connect@thanethomson.com> * Extract TryClone trait into its own crate As per the discussion at informalsystems/tendermint-rs#938 (comment), this extracts the `TryClone` trait into a new crate called `tendermint-std-ext` in the `std-ext` directory. This new crate is intended to contain any code that we need that extends the Rust standard library. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Reorder imports Signed-off-by: Thane Thomson <connect@thanethomson.com> * Assert validation regardless of debug build This introduces the internal encryption assertions at runtime regardless of build type. This may introduce a small performance hit, but it's probably worth it to ensure correctness. Effectively this is keeping an eye on the code in the `encrypt_and_write` fn to ensure its correctness. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove remote_pubkey optionality from sender/receiver halves Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update SecretConnection docs with comment content Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix doc link to TryClone trait Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add docs on SecretConnection failures and connection integrity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Synchronize sending/receiving failures to comply with crypto algorithm constraints Signed-off-by: Thane Thomson <connect@thanethomson.com> * Rename try_split method to split for SecretConnection Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove redundant field name prefixes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix broken link in docs Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix recent clippy errors on `master` (#941) * Fix needless borrows in codebase Signed-off-by: Thane Thomson <connect@thanethomson.com> * Ignore needless collect warning (we do actually seem to need it) Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove trailing semicolon in macro to fix docs compiling Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove unnecessary macros Signed-off-by: Thane Thomson <connect@thanethomson.com> * Correct error messages Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Thane Thomson <thane@informal.systems> Co-authored-by: Thane Thomson <connect@thanethomson.com>

thanethomson added 7 commits July 23, 2021 10:32

Implement thread-safe cloning of a secret connection

02a94f6

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Expand documentation for SecretConnection on threading considerations

31b8cad

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Extract peer construction into its own method

94c43c0

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add more messages to test

1409f91

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Expand comment for clarity

b161ba5

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add .changelog entry

b48fb23

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson requested review from xla and tarcieri July 23, 2021 17:36

thanethomson marked this pull request as ready for review July 23, 2021 17:36

informalsystems deleted a comment from codecov-commenter Jul 23, 2021

tony-iqlusion reviewed Jul 23, 2021

View reviewed changes

p2p/src/secret_connection.rs Outdated Show resolved Hide resolved

thanethomson marked this pull request as draft July 24, 2021 20:28

soareschen reviewed Jul 26, 2021

View reviewed changes

thanethomson added 9 commits July 26, 2021 11:02

Restore half-duplex operations

0a90dc3

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Extract encrypt/decrypt fns as independent methods

3c45316

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Remove unnecessary trait bounds

1d2f658

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Extract send/receive state

4c60492

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Extract read/write functionality as standalone methods

929eeb5

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add logic to facilitate splitting SecretConnection into its sending a…

465d627

…nd receiving halves Signed-off-by: Thane Thomson <connect@thanethomson.com>

Restore split SecretConnection test using new semantics

34a7de5

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Update changelog entry

b220c8f

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Update docs for SecretConnection

ba3b16c

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson marked this pull request as ready for review July 26, 2021 16:29

Condense error reporting

e65aeee

Signed-off-by: Thane Thomson <connect@thanethomson.com>

tony-iqlusion previously approved these changes Jul 26, 2021

View reviewed changes

xla reviewed Jul 27, 2021

View reviewed changes

thanethomson added 7 commits July 27, 2021 14:32

Remove remote_pubkey optionality from sender/receiver halves

042b3f4

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Update SecretConnection docs with comment content

c5be1b4

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix doc link to TryClone trait

9da6097

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix doc link to TryClone trait

2c9aafe

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add docs on SecretConnection failures and connection integrity

4f32c17

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Synchronize sending/receiving failures to comply with crypto algorith…

b8913c0

…m constraints Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson dismissed tony-iqlusion’s stale review via b8913c0 July 27, 2021 21:48

thanethomson requested review from xla and soareschen July 27, 2021 22:01

tony-iqlusion reviewed Jul 27, 2021

View reviewed changes

p2p/src/secret_connection.rs Outdated Show resolved Hide resolved

xla reviewed Jul 28, 2021

View reviewed changes

thanethomson added 2 commits July 28, 2021 10:58

Rename try_split method to split for SecretConnection

68ad278

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Remove redundant field name prefixes

a9c9b98

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix broken link in docs

2d5ef8b

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson requested review from xla and tony-iqlusion and removed request for tarcieri July 30, 2021 13:06

thanethomson changed the title ~~Implement thread-safe cloning of a secret connection~~ Implement full-duplex secret connection Aug 3, 2021

xla approved these changes Aug 4, 2021

View reviewed changes

thanethomson merged commit 09c8454 into master Aug 4, 2021

thanethomson deleted the thane/814-clonable-secret-conn branch August 4, 2021 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement full-duplex secret connection #938

Implement full-duplex secret connection #938

thanethomson commented Jul 23, 2021 •

edited

Loading

codecov-commenter commented Jul 23, 2021 •

edited

Loading

soareschen Jul 26, 2021

thanethomson Jul 26, 2021

thanethomson Jul 26, 2021

soareschen Jul 26, 2021

thanethomson Jul 26, 2021

soareschen Jul 26, 2021

thanethomson Jul 26, 2021

tony-iqlusion Jul 26, 2021

soareschen Jul 26, 2021

thanethomson Jul 26, 2021

soareschen Jul 26, 2021

thanethomson Jul 27, 2021

thanethomson commented Jul 26, 2021

tony-iqlusion left a comment

xla left a comment •

edited

Loading

xla Jul 27, 2021

thanethomson Jul 27, 2021

thanethomson commented Jul 27, 2021

thanethomson commented Jul 27, 2021

tony-iqlusion commented Jul 27, 2021

thanethomson commented Jul 27, 2021

xla left a comment

thanethomson commented Jul 28, 2021

xla left a comment

		debug_assert!(!chunk.is_empty(), "chunk is empty");
		debug_assert!(

Implement full-duplex secret connection #938

Implement full-duplex secret connection #938

Conversation

thanethomson commented Jul 23, 2021 • edited Loading

codecov-commenter commented Jul 23, 2021 • edited Loading

Codecov Report

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

thanethomson commented Jul 26, 2021

tony-iqlusion left a comment

Choose a reason for hiding this comment

xla left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thanethomson commented Jul 27, 2021

thanethomson commented Jul 27, 2021

tony-iqlusion commented Jul 27, 2021

thanethomson commented Jul 27, 2021

xla left a comment

Choose a reason for hiding this comment

thanethomson commented Jul 28, 2021

xla left a comment

Choose a reason for hiding this comment

thanethomson commented Jul 23, 2021 •

edited

Loading

codecov-commenter commented Jul 23, 2021 •

edited

Loading

xla left a comment •

edited

Loading