cleanup: remove redundant network port identifiers from maker logs an…#799
cleanup: remove redundant network port identifiers from maker logs an…#799Abhishek2634 wants to merge 6 commits intocitadel-tech:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR removes the maker's network port identifier: Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @mojoX911 I have completed the refactoring for the issue regarding redundant network port identifiers. Here was my approach to solving it: Remove the redundant port field from ThreadPool in api.rs and api2.rs. I have done runnig Thanks :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/maker/server.rs (1)
333-379: 🛠️ Refactor suggestion | 🟠 MajorKeep peer identity in the legacy server logs.
These messages now log connection lifecycle and message traffic with no peer or swap context. With concurrent legacy sessions,
Connection ended.,<=== ..., andError Handling client request ...become hard to attribute. Capturestream.peer_addr()once and include it in these logs.Also applies to: 517-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maker/server.rs` around lines 333 - 379, In handle_client, capture the client's peer address once at the start (call stream.peer_addr() and store it) and include that peer identifier in all subsequent logs within this function (e.g., the "Connection ended.", "<=== {}", "===> {}", and any error logs including the MakerError branches and IO send errors); update log lines around read_message, serde_cbor::from_slice, send_message, and the MakerError match so they prepend or interpolate the stored peer address to provide per-connection context while leaving existing control flow and error handling unchanged.src/maker/handlers.rs (1)
56-63: 🛠️ Refactor suggestion | 🟠 MajorKeep the swap id in the legacy handler logs.
Most of these legacy-path logs dropped the port prefix without replacing it with
id/message.id, even though that identifier is already available in the same scope. On a maker handling multiple legacy swaps, messages likeReserved ... UTXOs,Prepared outgoing funding txs, andBroadcasted funding txsare now ambiguous. Please follow the taproot pattern and carry the swap id through these log lines.Example pattern
- log::debug!("Reserved {} UTXOs for swap", connection_state.reserve_utxo.len(),); + log::debug!( + "[Swap: {}] Reserved {} UTXOs for swap", + message.id, + connection_state.reserve_utxo.len(), + );Based on learnings
Maker handler changes must be duplicated across legacy (\handlers.rs`) and Taproot (`handlers2.rs`) implementations`.Also applies to: 251-295, 327-484, 610-618, 650-699
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maker/handlers.rs` around lines 56 - 63, The legacy handler logs drop the swap identifier making messages ambiguous; update all log calls in src/maker/handlers.rs that handle legacy swap messages (including the block for TakerToMakerMessage::WaitingFundingConfirmation and the other ranges mentioned) to include the swap id/message.id like the Taproot handler does—e.g., change log::info!("Reserved ... UTXOs") / "Prepared outgoing funding txs" / "Broadcasted funding txs" to include the id variable (or message.id) in the log string, and propagate that id through the same code paths (where functions or closures reference id, use the existing id.clone() or message.id) so every legacy log line prints the swap id for unambiguous tracing.src/maker/api.rs (1)
919-934:⚠️ Potential issue | 🟠 MajorDo not log the full watch-response transaction at
info!.This path immediately inspects witness data for preimages, so dumping
transactionhere can persist the revealed secret and produces very noisy logs. Log the txid or an outpoint summary instead.Safer logging
- log::info!("Received WatchResponse with mempool txs: {:?}", transaction); + log::info!( + "Received WatchResponse for mempool tx {}", + transaction.compute_txid() + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maker/api.rs` around lines 919 - 934, The current info log prints the entire WatchResponse transaction (variable transaction) which can leak witness preimages; change the logging in the loop over responses to avoid serializing witness data — instead log safe identifiers such as transaction.txid (or a short outpoint summary built from each input's previous_output.txid and vout) and/or the input count; keep the rest of the logic that checks input.witness and calls update_swapcoins_with_preimages(maker, incomings, &preimages, true) / update_swapcoins_with_preimages(maker, outgoings, &preimages, false) unchanged, and ensure no witness bytes are included in the new log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/maker/api.rs`:
- Around line 608-617: The recovery logs currently emit generic messages and
must include stable identifiers so concurrent recoveries are traceable: update
the log! and log::info! calls in src/maker/api.rs (the blocks that reference
txid, failed_swap_ip, maker_clone and the spawned recovery thread) to include
the peer IP (ip), the swap id (use the swap identifier variable in scope, e.g.,
swap_id or similar), and the contract txid; do the same for the other recovery
log sites you noted (around lines ~690-698 and ~747-866) so every
warning/info/error emitted from the recovery path concatenates or formats ip,
swap id and txid consistently to identify which swap/peer triggered the message.
---
Outside diff comments:
In `@src/maker/api.rs`:
- Around line 919-934: The current info log prints the entire WatchResponse
transaction (variable transaction) which can leak witness preimages; change the
logging in the loop over responses to avoid serializing witness data — instead
log safe identifiers such as transaction.txid (or a short outpoint summary built
from each input's previous_output.txid and vout) and/or the input count; keep
the rest of the logic that checks input.witness and calls
update_swapcoins_with_preimages(maker, incomings, &preimages, true) /
update_swapcoins_with_preimages(maker, outgoings, &preimages, false) unchanged,
and ensure no witness bytes are included in the new log.
In `@src/maker/handlers.rs`:
- Around line 56-63: The legacy handler logs drop the swap identifier making
messages ambiguous; update all log calls in src/maker/handlers.rs that handle
legacy swap messages (including the block for
TakerToMakerMessage::WaitingFundingConfirmation and the other ranges mentioned)
to include the swap id/message.id like the Taproot handler does—e.g., change
log::info!("Reserved ... UTXOs") / "Prepared outgoing funding txs" /
"Broadcasted funding txs" to include the id variable (or message.id) in the log
string, and propagate that id through the same code paths (where functions or
closures reference id, use the existing id.clone() or message.id) so every
legacy log line prints the swap id for unambiguous tracing.
In `@src/maker/server.rs`:
- Around line 333-379: In handle_client, capture the client's peer address once
at the start (call stream.peer_addr() and store it) and include that peer
identifier in all subsequent logs within this function (e.g., the "Connection
ended.", "<=== {}", "===> {}", and any error logs including the MakerError
branches and IO send errors); update log lines around read_message,
serde_cbor::from_slice, send_message, and the MakerError match so they prepend
or interpolate the stored peer address to provide per-connection context while
leaving existing control flow and error handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cdfb9b67-33ff-47c8-bbe8-7cea6f6ef2cc
📒 Files selected for processing (7)
src/maker/api.rssrc/maker/api2.rssrc/maker/handlers.rssrc/maker/handlers2.rssrc/maker/rpc/server.rssrc/maker/server.rssrc/maker/server2.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/maker/api.rs`:
- Around line 882-892: The code computes anchor_id inside the loop even though
it is loop-invariant; move the anchor_id computation out of the loop in the
Maker timelock recovery logic (same fix as recover_via_hashlock) by computing
let anchor_id = outgoing.first().map(|c|
c.contract_tx.compute_txid().to_string()).unwrap_or_default(); once before the
loop that references broadcasted and outgoing, then use that anchor_id in the
log::info! call(s) instead of recomputing it each iteration.
- Around line 822-832: Compute anchor_id once before the loop instead of
recomputing inside each iteration: move the call to incoming.first().map(|c|
c.contract_tx.compute_txid().to_string()).unwrap_or_default() out of the loop
and store it in a local variable named anchor_id, then use that anchor_id in the
existing log::info! call (the log invocation that references "[Contract: {}]
Maker hashlock recovery: ..."). This avoids repeatedly calling compute_txid() on
the same incoming.first() value during each loop iteration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe478128-84c3-4ad1-8ff9-9760466357f9
📒 Files selected for processing (1)
src/maker/api.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/maker/api.rs`:
- Around line 873-881: The method chain indentation for calling
broadcast_outgoing_contracts on maker.wallet.write() is inconsistent; reformat
the chain so it matches the project's style (each chained call on its own
indented line) for the expression that assigns infos (the call sequence
maker.wallet.write()?.broadcast_outgoing_contracts(outgoing.clone())?) and
ensure the following anchor_id assignment using
outgoing.first().map(...).unwrap_or_default() remains aligned with surrounding
code (e.g., mirror the layout used in recover_via_hashlock).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35250f00-20bd-4a87-9f66-722c45b4e95c
📒 Files selected for processing (1)
src/maker/api.rs
Abhishek2634
force-pushed
the
cleanup/remove-network-port
branch
from
79e060c to
449c555
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/maker/handlers.rs (1)
56-63:⚠️ Potential issue | 🟡 MinorRestore the swap id in this timer-reset log.
idis already in scope, and without the old port prefix this line no longer tells you which swap's timer was touched.Suggested fix
- log::info!("Taker is waiting for funding confirmation. Resetting timer.",); + log::info!( + "[Swap: {}] Taker is waiting for funding confirmation. Resetting timer.", + id + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maker/handlers.rs` around lines 56 - 63, The log message inside the match arm for TakerToMakerMessage::WaitingFundingConfirmation should include the swap id so you know which timer was reset; update the log::info! call in that arm to print the id (e.g. log::info!("Taker is waiting for funding confirmation for swap {}. Resetting timer.", id);) before modifying maker.ongoing_swap_state.lock()? and returning Ok(None).
♻️ Duplicate comments (1)
src/maker/api.rs (1)
755-799:⚠️ Potential issue | 🟡 MinorKeep
anchor_txidon the follow-up recovery logs.
recover_from_swapcan run concurrently. After the initial[Contract: ...]line, thetimelock expired...andall preimages known...messages drop that anchor, so the recovery phase becomes ambiguous again.Suggested fix
if current_height >= timelock_expiry { log::info!( - "timelock expired at {} (expiry={}), using timelock path", + "[Contract: {}] timelock expired at {} (expiry={}), using timelock path", + anchor_txid, current_height, timelock_expiry ); return recover_via_timelock(maker, outgoing_swapcoins); } @@ if all_preimages_known { log::info!( - "all preimages known at height {}, using hashlock path", + "[Contract: {}] all preimages known at height {}, using hashlock path", + anchor_txid, current_height ); return recover_via_hashlock(maker, incoming_swapcoins); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maker/api.rs` around lines 755 - 799, The follow-up logs in recover_from_swap drop the anchor_txid context; update the two log::info calls that currently print "timelock expired..." and "all preimages known..." to include the anchor_txid (the variable defined earlier) in their format string (e.g. prefix with "[Contract: {}]" and pass anchor_txid) so those messages keep the same contract context as the initial log; ensure you modify the log::info invocations before calling recover_via_timelock and recover_via_hashlock respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/maker/handlers2.rs`:
- Around line 201-204: Update the two info logs that currently only print
swap_id so they include the protocol id (senders_contract.id) as well; e.g.
change the "Persisted incoming and outgoing swapcoins with swap_id={}" log to
include both senders_contract.id and swap_id in the message, and make the same
change for the other similar log at the nearby block (lines 207-210) so both
logs show "protocol_id={}" and "swap_id={}" together for easier
cross-referencing.
In `@src/maker/server.rs`:
- Around line 517-523: Accept currently throws away the peer address from
listener.accept(), which removes the per-session identifier and makes logs from
handle_client and its internal errors ambiguous; capture the peer address (e.g.,
let (mut stream, addr) = listener.accept() or rename to addr) and thread that
identifier into handle_client by updating its signature (e.g.,
handle_client(maker: &MakerType, stream: &mut TcpStream, peer: SocketAddr)) and
all internal logging sites so each log line (including "Connection ended", the
"<===", "===>", and error logs inside handle_client) includes the same peer
identifier for clear attribution across messages and sessions.
---
Outside diff comments:
In `@src/maker/handlers.rs`:
- Around line 56-63: The log message inside the match arm for
TakerToMakerMessage::WaitingFundingConfirmation should include the swap id so
you know which timer was reset; update the log::info! call in that arm to print
the id (e.g. log::info!("Taker is waiting for funding confirmation for swap {}.
Resetting timer.", id);) before modifying maker.ongoing_swap_state.lock()? and
returning Ok(None).
---
Duplicate comments:
In `@src/maker/api.rs`:
- Around line 755-799: The follow-up logs in recover_from_swap drop the
anchor_txid context; update the two log::info calls that currently print
"timelock expired..." and "all preimages known..." to include the anchor_txid
(the variable defined earlier) in their format string (e.g. prefix with
"[Contract: {}]" and pass anchor_txid) so those messages keep the same contract
context as the initial log; ensure you modify the log::info invocations before
calling recover_via_timelock and recover_via_hashlock respectively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e6c0dba-173e-4e06-9611-25fe739a62e6
📒 Files selected for processing (7)
src/maker/api.rssrc/maker/api2.rssrc/maker/handlers.rssrc/maker/handlers2.rssrc/maker/rpc/server.rssrc/maker/server.rssrc/maker/server2.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/maker/server.rs (1)
349-380:⚠️ Potential issue | 🟡 MinorKeep peer attribution consistent across all
handle_clienterror logs.Lines 349, 366, 376, and 379 still log without
[Peer: {}], so concurrent-session error attribution is still incomplete.🧭 Suggested fix
- log::error!("Net Error: {}", e); + log::error!("[Peer: {}] Net Error: {}", addr, e); @@ - log::error!("Closing due to IO error in sending message: {e:?}"); + log::error!( + "[Peer: {}] Closing due to IO error in sending message: {e:?}", + addr + ); @@ - log::error!("Maker Special Behavior Triggered Disconnection : {:?}", sp); + log::error!( + "[Peer: {}] Maker Special Behavior Triggered Disconnection: {:?}", + addr, + sp + ); @@ - log::error!("Internal message handling error occurred: {:?}", e); + log::error!( + "[Peer: {}] Internal message handling error occurred: {:?}", + addr, + e + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/maker/server.rs` around lines 349 - 380, Update the error log statements inside the handle_client loop to include the peer attribution like the other logs: prefix each log::error! at the locations that currently log without "[Peer: {}]" (the IO send error path around send_message, the MakerError::SpecialBehaviour branch, and the generic Err branch after handle_message) to use the same format string with "[Peer: {}]" and pass addr as the first formatter argument so all errors consistently show the peer (use the same logging style as the existing log::info!("[Peer: {}] ...", addr, ...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/maker/handlers2.rs`:
- Line 34: The debug log currently prints the entire TakerToMakerMessage (via
log::debug!("Handling message: {:?}", message)) which can leak secrets for the
PrivateKeyHandover variant; change the debug site to first convert the message
into a redacted form (e.g., match on TakerToMakerMessage and for
PrivateKeyHandover replace the sensitive payload with a fixed "[REDACTED]"
string or build a shallow redacted copy) and then log that redacted
representation instead; implement a small helper like
redacted_message_for_log(&TakerToMakerMessage) or a Display/Debug wrapper and
use it in the log::debug! call so only non-sensitive fields are printed while
PrivateKeyHandover payloads are omitted.
- Around line 64-71: After processing the taproot private key handover in
handle_privkey_handover, call Maker::sweep_after_successful_coinswap() and then
persist wallet state via maker.wallet().sync_and_save() (or the equivalent
sync_and_save method used in handlers.rs) before returning the
MakerToTakerMessage; specifically, after let response =
maker.process_private_key_handover(&privkey_handover, connection_state)? insert
a call to maker.sweep_after_successful_coinswap()? and then
maker.wallet().sync_and_save()? (or the corresponding methods on Maker) so the
taproot path mirrors the legacy handlers.rs behavior.
---
Duplicate comments:
In `@src/maker/server.rs`:
- Around line 349-380: Update the error log statements inside the handle_client
loop to include the peer attribution like the other logs: prefix each
log::error! at the locations that currently log without "[Peer: {}]" (the IO
send error path around send_message, the MakerError::SpecialBehaviour branch,
and the generic Err branch after handle_message) to use the same format string
with "[Peer: {}]" and pass addr as the first formatter argument so all errors
consistently show the peer (use the same logging style as the existing
log::info!("[Peer: {}] ...", addr, ...)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66b9f06b-98a9-4364-b73a-977a79569023
📒 Files selected for processing (2)
src/maker/handlers2.rssrc/maker/server.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
The part of the issue also described to find a new identifier for the makers. Just removing them is not enough. We need to add an alternate identifier in its place. |
2 participants
Pull Request
Description
Fixed redundant logging identifiers where the network port was used as a prefix. Removed the port field from ThreadPool as it was only used for logging.
Closes #798
Type of Change
Fix Screenshot
Integration tests passed
Protocol Version(s) Affected
messages.rs,contract.rs,handlers.rs)messages2.rs,contract2.rs,handlers2.rs)Affected Component(s)
docs/)Checklist
Code Quality
cargo +nightly fmt --alland committed the resultcargo +stable clippy --all-features --lib --bins --tests -- -D warningswith zero warningscargo +stable clippy --examples -- -D warningswith zero warningsRUSTDOCFLAGS="-D warnings" cargo +nightly doc --all-features --document-private-items --no-depswith zero warningsln -s ../../git_hooks/pre-commit .git/hooks/pre-commitif not already set)Testing
cargo test)cargo test --features integration-test)#[cfg(feature = "integration-test")]Documentation
docs/folder were updatedSecurity & Privacy (Critical)
integration-testfeature flag is not reachable in production code pathsHow to Test
Summary by CodeRabbit
Refactor
New Features
Chores