From f0ff3a18d20629b27af8d40f1ca042b4bd0f5d50 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Wed, 29 Nov 2023 00:24:37 -0500 Subject: [PATCH] Use debug builds in our Dockerfiles to reduce CI times (#462) * Use debug builds in our Dockerfiles to reduce CI times Also enables only spawning the mdns service when debug in the coordinator. * Correct underflow in processor Prior undetected due to relase builds not having bounds checks enabled. * Restore Serai release due to CI/RPC failures caused by compiling it in debug mode This is *probably* worth an issue filed upstream, if it can be tracked down. * Correct failing debug asserts in Monero These debug asserts assumed there was a change address to take the remainder. If there's no change address, the remainder is shunted to the fee, causing the fee to be distinct from the estimate. We presumably need to modify monero-serai such that change: None isn't valid, and users must use Change::Fingerprintable(None). --- coins/monero/src/wallet/send/mod.rs | 60 ++++++++++++------- coordinator/src/p2p.rs | 9 ++- orchestration/coordinator/Dockerfile | 4 +- .../coordinator/Dockerfile.coordinator | 4 +- orchestration/message-queue/Dockerfile | 4 +- .../message-queue/Dockerfile.message-queue | 4 +- orchestration/processor/bitcoin/Dockerfile | 4 +- .../bitcoin/Dockerfile.processor.bitcoin | 4 +- orchestration/processor/monero/Dockerfile | 4 +- .../monero/Dockerfile.processor.monero | 4 +- processor/src/main.rs | 7 ++- 11 files changed, 62 insertions(+), 46 deletions(-) diff --git a/coins/monero/src/wallet/send/mod.rs b/coins/monero/src/wallet/send/mod.rs index de7c0fe4..f770e270 100644 --- a/coins/monero/src/wallet/send/mod.rs +++ b/coins/monero/src/wallet/send/mod.rs @@ -314,6 +314,7 @@ pub struct SignableTransaction { protocol: Protocol, r_seed: Option>, inputs: Vec<(SpendableOutput, Decoys)>, + has_change: bool, payments: Vec, data: Vec>, fee: u64, @@ -385,7 +386,7 @@ fn need_additional(payments: &[InternalPayment]) -> (bool, bool) { (subaddresses, additional) } -fn sanity_check_change_payment(payments: &[InternalPayment], has_change_address: bool) { +fn sanity_check_change_payment_quantity(payments: &[InternalPayment], has_change_address: bool) { debug_assert_eq!( payments .iter() @@ -516,7 +517,7 @@ impl SignableTransaction { } // Sanity check we have the expected number of change outputs - sanity_check_change_payment(&payments, change_address.is_some()); + sanity_check_change_payment_quantity(&payments, change_address.is_some()); // Modify the amount of the change output if change_address.is_some() { @@ -527,23 +528,34 @@ impl SignableTransaction { } // Sanity check the change again after modifying - sanity_check_change_payment(&payments, change_address.is_some()); + sanity_check_change_payment_quantity(&payments, change_address.is_some()); // Sanity check outgoing amount + fee == incoming amount - debug_assert_eq!( - payments - .iter() - .map(|payment| match *payment { - InternalPayment::Payment(payment) => payment.1, - InternalPayment::Change(_, amount) => amount, - }) - .sum::() + - fee, - in_amount, - "Outgoing amount + fee != incoming amount" - ); + if change_address.is_some() { + debug_assert_eq!( + payments + .iter() + .map(|payment| match *payment { + InternalPayment::Payment(payment) => payment.1, + InternalPayment::Change(_, amount) => amount, + }) + .sum::() + + fee, + in_amount, + "Outgoing amount + fee != incoming amount" + ); + } - Ok(SignableTransaction { protocol, r_seed, inputs, payments, data, fee, fee_rate }) + Ok(SignableTransaction { + protocol, + r_seed, + inputs, + payments, + has_change: change_address.is_some(), + data, + fee, + fee_rate, + }) } pub fn fee(&self) -> u64 { @@ -778,7 +790,9 @@ impl SignableTransaction { }); encrypted_amounts.push(EncryptedAmount::Compact { amount: output.amount }); } - debug_assert_eq!(self.fee, fee, "transaction will use an unexpected fee"); + if self.has_change { + debug_assert_eq!(self.fee, fee, "transaction will use an unexpected fee"); + } ( Transaction { @@ -844,11 +858,13 @@ impl SignableTransaction { _ => unreachable!("attempted to sign a TX which wasn't CLSAG"), } - debug_assert_eq!( - self.fee_rate.calculate_fee_from_weight(tx.weight()), - tx.rct_signatures.base.fee, - "transaction used unexpected fee", - ); + if self.has_change { + debug_assert_eq!( + self.fee_rate.calculate_fee_from_weight(tx.weight()), + tx.rct_signatures.base.fee, + "transaction used unexpected fee", + ); + } Ok(tx) } diff --git a/coordinator/src/p2p.rs b/coordinator/src/p2p.rs index aa392c52..f3dda43b 100644 --- a/coordinator/src/p2p.rs +++ b/coordinator/src/p2p.rs @@ -191,7 +191,7 @@ pub trait P2p: Send + Sync + Clone + fmt::Debug + TributaryP2p { #[derive(NetworkBehaviour)] struct Behavior { gossipsub: GsBehavior, - //#[cfg(debug_assertions)] + #[cfg(debug_assertions)] mdns: libp2p::mdns::tokio::Behaviour, } @@ -261,8 +261,7 @@ impl LibP2p { }, // Only use MDNS in debug environments, as it should have no value in a release build - // TODO: We do tests on release binaries as of right now... - //#[cfg(debug_assertions)] + #[cfg(debug_assertions)] mdns: { log::info!("creating mdns service"); libp2p::mdns::tokio::Behaviour::new(libp2p::mdns::Config::default(), throwaway_peer_id) @@ -371,7 +370,7 @@ impl LibP2p { // Handle new incoming messages event = swarm.next() => { match event { - //#[cfg(debug_assertions)] + #[cfg(debug_assertions)] Some(SwarmEvent::Behaviour(BehaviorEvent::Mdns( libp2p::mdns::Event::Discovered(list), ))) => { @@ -383,7 +382,7 @@ impl LibP2p { } } } - //#[cfg(debug_assertions)] + #[cfg(debug_assertions)] Some(SwarmEvent::Behaviour(BehaviorEvent::Mdns( libp2p::mdns::Event::Expired(list), ))) => { diff --git a/orchestration/coordinator/Dockerfile b/orchestration/coordinator/Dockerfile index edace88c..44be3116 100644 --- a/orchestration/coordinator/Dockerfile +++ b/orchestration/coordinator/Dockerfile @@ -46,8 +46,8 @@ RUN --mount=type=cache,target=/root/.cargo \ --mount=type=cache,target=/usr/local/cargo/git \ --mount=type=cache,target=/serai/target \ mkdir /serai/bin && \ - cargo build -p serai-coordinator --release --all-features && \ - mv /serai/target/release/serai-coordinator /serai/bin + cargo build -p serai-coordinator --all-features && \ + mv /serai/target/debug/serai-coordinator /serai/bin FROM debian:bookworm-slim as image COPY --from=mimalloc libmimalloc.so /usr/lib diff --git a/orchestration/coordinator/Dockerfile.coordinator b/orchestration/coordinator/Dockerfile.coordinator index a41506c8..c3cbbb45 100644 --- a/orchestration/coordinator/Dockerfile.coordinator +++ b/orchestration/coordinator/Dockerfile.coordinator @@ -1,2 +1,2 @@ - cargo build -p serai-coordinator --release --all-features && \ - mv /serai/target/release/serai-coordinator /serai/bin + cargo build -p serai-coordinator --all-features && \ + mv /serai/target/debug/serai-coordinator /serai/bin diff --git a/orchestration/message-queue/Dockerfile b/orchestration/message-queue/Dockerfile index d9728dc8..c5ea8a6d 100644 --- a/orchestration/message-queue/Dockerfile +++ b/orchestration/message-queue/Dockerfile @@ -46,8 +46,8 @@ RUN --mount=type=cache,target=/root/.cargo \ --mount=type=cache,target=/usr/local/cargo/git \ --mount=type=cache,target=/serai/target \ mkdir /serai/bin && \ - cargo build --release --all-features -p serai-message-queue && \ - mv /serai/target/release/serai-message-queue /serai/bin + cargo build --all-features -p serai-message-queue && \ + mv /serai/target/debug/serai-message-queue /serai/bin FROM debian:bookworm-slim as image COPY --from=mimalloc libmimalloc.so /usr/lib diff --git a/orchestration/message-queue/Dockerfile.message-queue b/orchestration/message-queue/Dockerfile.message-queue index 320a1cea..beb69720 100644 --- a/orchestration/message-queue/Dockerfile.message-queue +++ b/orchestration/message-queue/Dockerfile.message-queue @@ -1,2 +1,2 @@ - cargo build --release --all-features -p serai-message-queue && \ - mv /serai/target/release/serai-message-queue /serai/bin + cargo build --all-features -p serai-message-queue && \ + mv /serai/target/debug/serai-message-queue /serai/bin diff --git a/orchestration/processor/bitcoin/Dockerfile b/orchestration/processor/bitcoin/Dockerfile index 0d06f07f..f7d1c9c1 100644 --- a/orchestration/processor/bitcoin/Dockerfile +++ b/orchestration/processor/bitcoin/Dockerfile @@ -46,8 +46,8 @@ RUN --mount=type=cache,target=/root/.cargo \ --mount=type=cache,target=/usr/local/cargo/git \ --mount=type=cache,target=/serai/target \ mkdir /serai/bin && \ - cargo build --release --features "binaries bitcoin" -p serai-processor && \ - mv /serai/target/release/serai-processor /serai/bin + cargo build --features "binaries bitcoin" -p serai-processor && \ + mv /serai/target/debug/serai-processor /serai/bin FROM debian:bookworm-slim as image COPY --from=mimalloc libmimalloc.so /usr/lib diff --git a/orchestration/processor/bitcoin/Dockerfile.processor.bitcoin b/orchestration/processor/bitcoin/Dockerfile.processor.bitcoin index 36393e0a..9ed38e03 100644 --- a/orchestration/processor/bitcoin/Dockerfile.processor.bitcoin +++ b/orchestration/processor/bitcoin/Dockerfile.processor.bitcoin @@ -1,2 +1,2 @@ - cargo build --release --features "binaries bitcoin" -p serai-processor && \ - mv /serai/target/release/serai-processor /serai/bin + cargo build --features "binaries bitcoin" -p serai-processor && \ + mv /serai/target/debug/serai-processor /serai/bin diff --git a/orchestration/processor/monero/Dockerfile b/orchestration/processor/monero/Dockerfile index 1f33967f..cf531aa9 100644 --- a/orchestration/processor/monero/Dockerfile +++ b/orchestration/processor/monero/Dockerfile @@ -46,8 +46,8 @@ RUN --mount=type=cache,target=/root/.cargo \ --mount=type=cache,target=/usr/local/cargo/git \ --mount=type=cache,target=/serai/target \ mkdir /serai/bin && \ - cargo build --release --features "binaries monero" -p serai-processor && \ - mv /serai/target/release/serai-processor /serai/bin + cargo build --features "binaries monero" -p serai-processor && \ + mv /serai/target/debug/serai-processor /serai/bin FROM debian:bookworm-slim as image COPY --from=mimalloc libmimalloc.so /usr/lib diff --git a/orchestration/processor/monero/Dockerfile.processor.monero b/orchestration/processor/monero/Dockerfile.processor.monero index f11f50bb..31d0a811 100644 --- a/orchestration/processor/monero/Dockerfile.processor.monero +++ b/orchestration/processor/monero/Dockerfile.processor.monero @@ -1,2 +1,2 @@ - cargo build --release --features "binaries monero" -p serai-processor && \ - mv /serai/target/release/serai-processor /serai/bin + cargo build --features "binaries monero" -p serai-processor && \ + mv /serai/target/debug/serai-processor /serai/bin diff --git a/processor/src/main.rs b/processor/src/main.rs index 799e528f..8c63b2ae 100644 --- a/processor/src/main.rs +++ b/processor/src/main.rs @@ -569,12 +569,13 @@ async fn run(mut raw_db: D, network: N, mut // KeyGen specifically may take a notable amount of processing time // While that shouldn't be an issue in practice, as after processing an attempt it'll handle // the other messages in the queue, it may be beneficial to parallelize these - // They could likely be parallelized by type (KeyGen, Sign, Substrate) without issue + // They could potentially be parallelized by type (KeyGen, Sign, Substrate) without issue msg = coordinator.recv() => { - assert_eq!(msg.id, (last_coordinator_msg.unwrap_or(msg.id - 1) + 1)); + if let Some(last_coordinator_msg) = last_coordinator_msg { + assert_eq!(msg.id, last_coordinator_msg + 1); + } last_coordinator_msg = Some(msg.id); - // Only handle this if we haven't already if HandledMessageDb::get(&main_db, msg.id).is_none() { HandledMessageDb::set(&mut txn, msg.id, &());