ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-07-27 09:00:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

code review comments

Browse Source 
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
This commit is contained in:
Pratik Mankawde
2026-07-06 20:40:12 +01:00
parent 33c17e50b3
commit 38f6afbf4c
13 changed files with 79 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
.github/scripts/levelization/results/loops.txt vendored
View File

@@ -13,9 +13,6 @@ Loop: xrpld.app xrpld.rpc
Loop: xrpld.app xrpld.shamap
xrpld.shamap > xrpld.app
Loop: xrpld.app xrpld.telemetry
xrpld.telemetry ~= xrpld.app
Loop: xrpld.overlay xrpld.rpc
xrpld.rpc ~= xrpld.overlay

1
.github/scripts/levelization/results/ordering.txt vendored
View File

@@ -261,6 +261,7 @@ xrpld.app > xrpl.config
xrpld.app > xrpl.core
xrpld.app > xrpld.consensus
xrpld.app > xrpld.core
xrpld.app > xrpld.telemetry
xrpld.app > xrpl.json
xrpld.app > xrpl.ledger
xrpld.app > xrpl.net

50
OpenTelemetryPlan/Phase3_taskList.md
View File

@@ -166,7 +166,8 @@
## Task 3.6: Context Propagation in Transaction Relay
**Status**: COMPLETE
**Status**: COMPLETE (transaction relay). Consensus proposal/validation
propagation is deferred to Phase 4 — see "Planned (Phase 4)" below.
**Objective**: Ensure trace context flows correctly when transactions are relayed between peers, creating linked spans across nodes.
@@ -178,17 +179,6 @@
wired in PeerImp) extracts the parent span_id and creates the tx.receive span
as a child of the sender's tx.process span.
- **Proposal send/receive**: `RCLConsensus::Adaptor::propose()` injects the
current thread's active span context into the `TMProposeSet` protobuf via
`telemetry::injectToProtobuf()`. PeerImp creates a
`consensus.proposal.receive` span that extracts the sender's trace context
as parent (via `ConsensusReceiveTracing.h`).
- **Validation send/receive**: `RCLConsensus::Adaptor::validate()` injects
the current thread's active span context into the `TMValidation` protobuf.
PeerImp creates a `consensus.validation.receive` span that extracts the
sender's trace context as parent.
- **Edge cases**: Missing trace context (older peers) degrades gracefully to
standalone spans. Invalid/corrupted context is treated as absent. Trace
flags are propagated and respected.
@@ -200,20 +190,36 @@
- `PropagationHelpers.h` — `injectSpanContext(SpanGuard&, proto)` bridge
between SpanGuard and protobuf TraceContext.
- `TraceContextPropagator.h` — `injectToProtobuf(ctx, proto)` for
same-thread injection via OTel RuntimeContext (used in propose/validate).
- `ConsensusReceiveTracing.h` — `proposalReceiveSpan()` and
`validationReceiveSpan()` helper functions that create receive spans with
optional parent context extraction from incoming protobuf messages.
same-thread injection via OTel RuntimeContext.
**Key modified files**:
- `src/xrpld/app/misc/NetworkOPs.cpp` — tx relay injection
- `src/xrpld/app/consensus/RCLConsensus.cpp` — proposal/validation send injection
- `src/xrpld/overlay/detail/PeerImp.cpp` — proposal/validation receive spans
- `include/xrpl/telemetry/SpanGuard.h` — `TraceBytes` struct, `getTraceBytes()`
- `src/libxrpl/telemetry/SpanGuard.cpp` — `getTraceBytes()` implementation
- `src/xrpld/telemetry/PropagationHelpers.h` — inject helpers (new file)
- `src/xrpld/telemetry/ConsensusReceiveTracing.h` — receive span helpers (new file)
**Planned (Phase 4 — not in this PR)**:
The consensus proposal/validation propagation below is Phase 4 scope and is
not implemented on this branch. It is listed here only to record the intended
design.
- **Proposal send/receive**: `RCLConsensus::Adaptor::propose()` injects the
current thread's active span context into the `TMProposeSet` protobuf via
`telemetry::injectToProtobuf()`. PeerImp creates a
`consensus.proposal.receive` span that extracts the sender's trace context
as parent (via `ConsensusReceiveTracing.h`).
- **Validation send/receive**: `RCLConsensus::Adaptor::validate()` injects
the current thread's active span context into the `TMValidation` protobuf.
PeerImp creates a `consensus.validation.receive` span that extracts the
sender's trace context as parent.
- Planned files: `src/xrpld/app/consensus/RCLConsensus.cpp` (send injection),
`src/xrpld/overlay/detail/PeerImp.cpp` (receive spans),
`src/xrpld/telemetry/ConsensusReceiveTracing.h` (receive span helpers,
new file).
**Reference**:
@@ -396,7 +402,7 @@ This gives the best of both worlds: guaranteed cross-node correlation via determ
- `src/xrpld/overlay/detail/PeerImp.cpp` — restructured span creation
- `src/xrpld/app/misc/NetworkOPs.cpp` — deterministic context for tx.process
- `src/xrpld/app/misc/TxSpanNames.h` — new `traceStrategy` attribute constant
- `src/xrpld/telemetry/TxSpanNames.h` — new `traceStrategy` attribute constant
- New or shared utility for `createDeterministicTxContext()` (location TBD: could be
a shared header like `include/xrpl/telemetry/DeterministicContext.h`, or file-local
if only used in two places)
@@ -431,7 +437,7 @@ This gives the best of both worlds: guaranteed cross-node correlation via determ
- `txSpan(category, group, name, txHash, parentCtx)` — child span (deterministic
trace_id combined with protobuf-extracted parent span_id for relay ordering).
- **`TxTracing.h` helper functions** (`src/xrpld/overlay/detail/TxTracing.h`):
- **`TxTracing.h` helper functions** (`src/xrpld/telemetry/TxTracing.h`):
File-local helpers that wrap `SpanGuard::txSpan()` for the two main PeerImp call
sites:
- `txReceiveSpan(txHash, parentCtx)` — creates `tx.receive` span with
@@ -495,7 +501,7 @@ This gives the best of both worlds: guaranteed cross-node correlation via determ
**Modified files**:
- `src/xrpld/app/misc/TxSpanNames.h`
- `src/xrpld/telemetry/TxSpanNames.h`
- `src/xrpld/app/misc/detail/TxQSpanNames.h`
- `src/xrpld/app/misc/NetworkOPs.cpp`
- `src/xrpld/overlay/detail/PeerImp.cpp`

19
OpenTelemetryPlan/secure-OTel.md
View File

@@ -20,7 +20,7 @@ The most effective way to prevent spoofing from external sources is to treat you
server {
listen 80;
location {
location / {
# Clear out untrusted incoming trace headers
proxy_set_header traceparent "";
proxy_set_header tracestate "";
@@ -92,10 +92,11 @@ receivers:
grpc:
endpoint: 0.0.0.0:4317
tls:
# Setting client_ca_file makes the collector require and verify a
# client cert, rejecting connections without a trusted one.
client_ca_file: /certs/client_ca.pem # CA that signs trusted client certs
cert_file: /certs/collector.pem
key_file: /certs/collector.key
auth_type: require_and_verify_client_cert # Rejects unauthorized clients
```
### **Approach C: Application Layer Authentication (Basic Auth Extension)**
@@ -175,7 +176,7 @@ xrpld has **two distinct attack surfaces**, not one. The original guide conflate
│ │ OTLP/gRPC
│ │ + mTLS
│ ▼
└───────────────────────────────────────── [require_and_verify_client_cert]
└───────────────────────────────────────── [client_ca_file: verify client cert]
OTel Collector
(in private subnet, NetPol)
```
@@ -196,15 +197,15 @@ The guide's NGINX header stripping and OTTL stale-span filtering target HTTP gat
Evaluated for the across-network deployment shape:
| Approach | Across-network fit | Cost | Verdict |
| ------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------- | ---------------------------------- |
| **A. NetworkPolicy / firewall** | Necessary baseline (don't expose `4317`/`4318` to the internet), but insufficient on its own when traffic genuinely crosses networks — you cannot NetworkPolicy the public internet. | Cheap. | **Defense-in-depth, not primary.** |
| **B. mTLS** | Strongest fit. Every xrpld node holds a client cert; collector verifies with `require_and_verify_client_cert`. Encrypts in transit (raw OTLP over the internet leaks transaction patterns and validator identity). Compromised node = revoke one cert, no shared secret to rotate everywhere. | Cert issuance + rotation pipeline. | **Primary.** |
| **C. Basic Auth** | Worst shape for this topology. Single shared password across all xrpld nodes — one leaked node config compromises the whole fleet. Doesn't encrypt; you'd need TLS underneath anyway, at which point you're 80% of the way to mTLS. | Cheap to set up, expensive to operate (rotation across N operators). | **Skip.** |
| Approach | Across-network fit | Cost | Verdict |
| ------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------- | ---------------------------------- |
| **A. NetworkPolicy / firewall** | Necessary baseline (don't expose `4317`/`4318` to the internet), but insufficient on its own when traffic genuinely crosses networks — you cannot NetworkPolicy the public internet. | Cheap. | **Defense-in-depth, not primary.** |
| **B. mTLS** | Strongest fit. Every xrpld node holds a client cert; the collector verifies it via `client_ca_file` in the receiver's `tls` block. Encrypts in transit (raw OTLP over the internet leaks transaction patterns and validator identity). Compromised node = revoke one cert, no shared secret to rotate everywhere. | Cert issuance + rotation pipeline. | **Primary.** |
| **C. Basic Auth** | Worst shape for this topology. Single shared password across all xrpld nodes — one leaked node config compromises the whole fleet. Doesn't encrypt; you'd need TLS underneath anyway, at which point you're 80% of the way to mTLS. | Cheap to set up, expensive to operate (rotation across N operators). | **Skip.** |
## Decision
**Primary defense:** mTLS (Approach B) on the collector's OTLP receivers, with `auth_type: require_and_verify_client_cert`.
**Primary defense:** mTLS (Approach B) on the collector's OTLP receivers. The collector requires and verifies each client certificate when `client_ca_file` is set in the receiver's `tls` block (there is no `auth_type` field — setting `client_ca_file` is what enforces client-cert verification).
**Defense-in-depth:** NetworkPolicy / firewall rules (Approach A) so `4317`/`4318` are never reachable from outside the expected operator subnets even if mTLS were misconfigured.

11
include/xrpl/tx/detail/TxApplySpanNames.h
View File

@@ -51,9 +51,11 @@
* @endcode
*
* @code
* // Transactor::operator() uses span() with prefix + suffix:
* auto span = SpanGuard::span(
* TraceCategory::Transactions, seg::tx, tx_apply_span::op::transactor);
* // Transactor::operator() also uses hashSpan on the same txID so it
* // co-traces with preflight and preclaim under one trace_id:
* auto span = SpanGuard::hashSpan(
* TraceCategory::Transactions, tx_apply_span::transactor,
* txID.data(), txID.kBytes);
* span.setAttribute(tx_apply_span::attr::stage, tx_apply_span::val::apply);
* @endcode
*/
@@ -79,6 +81,9 @@ inline constexpr auto transactor = makeStr("transactor");
inline constexpr auto preflight = join(seg::tx, op::preflight);
/// "tx.preclaim" — full name for hashSpan() at the preclaim stage.
inline constexpr auto preclaim = join(seg::tx, op::preclaim);
/// "tx.transactor" — full name for hashSpan() at the apply stage. Shares the
/// txID-derived trace_id so it co-traces with tx.preflight and tx.preclaim.
inline constexpr auto transactor = join(seg::tx, op::transactor);
// ===== Attribute keys ======================================================

16
src/libxrpl/tx/Transactor.cpp
View File

@@ -1294,10 +1294,15 @@ Transactor::checkInvariants(TER result, XRPAmount fee)
ApplyResult
Transactor::operator()()
{
auto span = telemetry::SpanGuard::span(
// Derive the trace_id from the transaction id so this apply-stage span
// shares one trace with the preflight and preclaim spans (which also use
// hashSpan on the same id).
auto const txID = ctx_.tx.getTransactionID();
auto span = telemetry::SpanGuard::hashSpan(
telemetry::TraceCategory::Transactions,
telemetry::seg::tx,
telemetry::tx_apply_span::op::transactor);
telemetry::tx_apply_span::transactor,
txID.data(),
txID.kBytes);
// "apply" — the third apply-pipeline stage, after preflight and preclaim.
span.setAttribute(telemetry::tx_apply_span::attr::stage, telemetry::tx_apply_span::val::apply);
if (auto const* fmt = TxFormats::getInstance().findByType(ctx_.tx.getTxnType()))
@@ -1428,6 +1433,11 @@ Transactor::operator()()
span.setAttribute(telemetry::tx_apply_span::attr::terResult, transToken(result).c_str());
span.setAttribute(telemetry::tx_apply_span::attr::applied, applied);
// Mark the span as errored when the transaction was not applied or the
// engine result is not a success, so failed applies surface in span-status
// error counts alongside preflight and preclaim.
if (!applied || !isTesSuccess(result))
span.setError(transToken(result));
return {result, applied, metadata};
}

4
src/libxrpl/tx/applySteps.cpp
View File

@@ -190,6 +190,10 @@ invokePreflight(PreflightContext const& ctx)
{
span.setAttribute(
telemetry::tx_apply_span::attr::terResult, transToken(result.first).c_str());
// Mark the span as errored when preflight rejects the transaction so
// failed stages surface in span-status error counts.
if (!isTesSuccess(result.first))
span.setError(transToken(result.first));
}
return result;
}

18
src/tests/libxrpl/telemetry/TraceContextPropagator.cpp
View File

@@ -81,10 +81,8 @@ TEST(TraceContextPropagator, extract_empty_protobuf)
protocol::TraceContext const proto;
auto ctx = xrpl::telemetry::extractFromProtobuf(proto);
auto span = trace::GetSpan(ctx);
if (span)
{
EXPECT_FALSE(span->GetContext().IsValid());
}
ASSERT_NE(span, nullptr);
EXPECT_FALSE(span->GetContext().IsValid());
}
TEST(TraceContextPropagator, extract_wrong_size_trace_id)
@@ -95,10 +93,8 @@ TEST(TraceContextPropagator, extract_wrong_size_trace_id)
auto ctx = xrpl::telemetry::extractFromProtobuf(proto);
auto span = trace::GetSpan(ctx);
if (span)
{
EXPECT_FALSE(span->GetContext().IsValid());
}
ASSERT_NE(span, nullptr);
EXPECT_FALSE(span->GetContext().IsValid());
}
TEST(TraceContextPropagator, extract_wrong_size_span_id)
@@ -109,10 +105,8 @@ TEST(TraceContextPropagator, extract_wrong_size_span_id)
auto ctx = xrpl::telemetry::extractFromProtobuf(proto);
auto span = trace::GetSpan(ctx);
if (span)
{
EXPECT_FALSE(span->GetContext().IsValid());
}
ASSERT_NE(span, nullptr);
EXPECT_FALSE(span->GetContext().IsValid());
}
TEST(TraceContextPropagator, inject_invalid_span)

2
src/xrpld/app/misc/NetworkOPs.cpp
View File

@@ -17,7 +17,6 @@
#include <xrpld/app/misc/FeeVote.h>
#include <xrpld/app/misc/Transaction.h>
#include <xrpld/app/misc/TxQ.h>
#include <xrpld/app/misc/TxSpanNames.h>
#include <xrpld/app/misc/ValidatorKeys.h>
#include <xrpld/app/misc/ValidatorList.h>
#include <xrpld/app/misc/make_NetworkOPs.h>
@@ -35,6 +34,7 @@
#include <xrpld/rpc/MPTokenIssuanceID.h>
#include <xrpld/rpc/ServerHandler.h>
#include <xrpld/telemetry/PropagationHelpers.h>
#include <xrpld/telemetry/TxSpanNames.h>
#include <xrpld/telemetry/TxTracing.h>
#include <xrpl/basics/Log.h>

9
src/xrpld/app/misc/detail/TxQ.cpp
View File

@@ -1449,9 +1449,6 @@ bool
TxQ::accept(Application& app, OpenView& view)
{
using namespace telemetry;
auto span =
SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::accept);
span.setAttribute(txq_span::attr::queueSize, static_cast<int64_t>(byFee_.size()));
/* Move transactions from the queue from largest fee level to smallest.
As we add more transactions, the required fee level will increase.
@@ -1463,6 +1460,12 @@ TxQ::accept(Application& app, OpenView& view)
std::scoped_lock const lock(mutex_);
// Create the span and read byFee_.size() only after taking the lock, since
// byFee_ is guarded by mutex_.
auto span =
SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::accept);
span.setAttribute(txq_span::attr::queueSize, static_cast<int64_t>(byFee_.size()));
auto const metricsSnapshot = feeMetrics_.getSnapshot();
for (auto candidateIter = byFee_.begin(); candidateIter != byFee_.end();)

2
src/xrpld/overlay/detail/PeerImp.cpp
View File

@@ -7,7 +7,6 @@
#include <xrpld/app/ledger/LedgerMaster.h>
#include <xrpld/app/ledger/TransactionMaster.h>
#include <xrpld/app/misc/Transaction.h>
#include <xrpld/app/misc/TxSpanNames.h>
#include <xrpld/app/misc/ValidatorList.h>
#include <xrpld/consensus/Validations.h>
#include <xrpld/overlay/Cluster.h>
@@ -22,6 +21,7 @@
#include <xrpld/overlay/detail/Tuning.h>
#include <xrpld/peerfinder/PeerfinderManager.h>
#include <xrpld/peerfinder/Slot.h>
#include <xrpld/telemetry/TxSpanNames.h>
#include <xrpld/telemetry/TxTracing.h>
#include <xrpl/basics/Blob.h>

0
src/xrpld/app/misc/TxSpanNames.h → src/xrpld/telemetry/TxSpanNames.h
View File

2
src/xrpld/telemetry/TxTracing.h
View File

@@ -10,7 +10,7 @@
* no-op SpanGuard instances (zero overhead, zero dependencies).
*/
#include <xrpld/app/misc/TxSpanNames.h>
#include <xrpld/telemetry/TxSpanNames.h>
#include <xrpl/basics/base_uint.h>
#include <xrpl/proto/xrpl.pb.h>