fix(rs-dapi,sdk): decode base64 CBOR error messages and detect duplicate identity keys#3350
fix(rs-dapi,sdk): decode base64 CBOR error messages and detect duplicate identity keys#3350
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracts embedded CBOR/base64 "message" fields from Tenderdash JSON Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DapiJSON as Tenderdash JSON handler
participant DapiGRPC as DapiClient error converter
participant Metadata as gRPC Metadata / JSON "data"
participant Decoder as CBOR/Base64 Decoder
participant ErrorEnum as Error Mapper
Client->>DapiJSON: receive JSON error (contains "message" and/or "data")
DapiJSON->>Metadata: inspect top-level "message" and "data"
alt top-level "message" missing/empty or "Internal error"
DapiJSON->>Decoder: attempt permissive base64 -> CBOR decode of "data"
Decoder-->>DapiJSON: optional "message" string
DapiJSON->>ErrorEnum: map using decoded message or fallback raw "data"
else top-level "message" present
DapiJSON->>ErrorEnum: map using top-level "message"
end
Client->>DapiGRPC: receive gRPC error (Status + metadata)
DapiGRPC->>Metadata: inspect `drive-error-data-bin` when Code::Internal
alt metadata present
DapiGRPC->>Decoder: CBOR-decode metadata bytes
Decoder-->>DapiGRPC: optional "message" string
DapiGRPC->>ErrorEnum: return Error::DriveInternalError(message) if found
else
DapiGRPC->>ErrorEnum: continue existing mapping or fallback
end
ErrorEnum-->>Client: mapped Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Improves dash-sdk’s error conversion so callers can recover structured ConsensusError information even when DAPI does not provide the primary dash-serialized-consensus-error-bin metadata, by falling back to decoding drive-error-data-bin.
Changes:
- Add a fallback path in
From<DapiClientError> for Errorto parse CBORdrive-error-data-binand extract embeddedconsensus_errorbytes. - Add explicit defensive limits (8 KiB max metadata size, CBOR recursion limit 16) before deserializing untrusted metadata.
- Add unit tests covering the fallback behavior and precedence.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lklimek
force-pushed
the
fix/sdk-cbor-security-hardening
branch
from
b173ca5 to
4aa4910
Compare
When Tenderdash returns an error with `message: "Internal error"`, rs-dapi falls through to the `data` field. Previously, the raw base64 string was stored as the message, producing unintelligible gRPC status messages like "oWdtZXNzYWdleMVzdG9yYWdl...". Now `decode_data_message()` tries to decode the `data` field as base64 → CBOR and extract the human-readable `message` text. If decoding fails (plain text data), the raw string is preserved as before. This fixes the 99% case from production DET logs: GroveDB duplicate-key errors (code 13) where the actual message "storage: identity: a unique key with that hash already exists..." was buried in double-encoded CBOR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
changed the title
lklimek
force-pushed
the
fix/sdk-cbor-security-hardening
branch
from
0fc9aee to
bfba2ef
Compare
There was a problem hiding this comment.
Pull request overview
Improves rs-dapi’s Tenderdash JSON-RPC error mapping by decoding base64-encoded CBOR stored in the Tenderdash data field (when the top-level message is unhelpful like "Internal error"), so gRPC clients receive human-readable error messages instead of base64 gibberish.
Changes:
- Added
decode_data_message()helper to decodedataas base64 → CBOR and extract the"message"field. - Updated
TenderdashStatus::from(serde_json::Value)to prefer decodeddata.messagewhen appropriate, while preserving prior fallback behavior. - Added unit tests for decoding behavior plus real-world DET log fixtures.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/rs-dapi/src/services/platform_service/error_mapping.rs
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3350 +/- ##
============================================
+ Coverage 79.93% 79.94% +0.01%
============================================
Files 2861 2861
Lines 280230 280352 +122
============================================
+ Hits 223993 224121 +128
+ Misses 56237 56231 -6
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-scoped PR that adds base64→CBOR decoding for the Tenderdash error.data field, with good test coverage including real-world DET log fixtures. The new decode_data_message() function follows existing patterns in the module, reuses base64_decode() and walk_cbor_for_key(), and gracefully falls back on any decode failure. No blocking issues. Two suggestions worth considering: the pre-existing is_ascii() filter silently drops non-ASCII UTF-8 data strings (contradicting the 'raw string preserved' claim), and the other Tenderdash error ingestion paths (wait_for_state_transition_result) still pass data through without attempting CBOR decoding.
Reviewed commit: 7ae8846
🟡 3 suggestion(s)
1 additional finding
🟡 suggestion: Other Tenderdash error paths still pass `data` through without CBOR decoding
packages/rs-dapi/src/services/platform_service/wait_for_state_transition_result.rs (lines 151-161)
The new decode_data_message() is only invoked inside impl From<serde_json::Value>, which handles JSON-RPC error responses. The wait_for_state_transition_result path constructs TenderdashStatus::new() directly from ABCI tx_result.data (lines 151–161, 243) without attempting CBOR decoding.
This may be intentional — the ABCI responses could use a different encoding format than JSON-RPC errors. But if they can also carry the same base64-CBOR payloads, clients would still receive unreadable strings on those paths. Worth investigating whether TenderdashStatus::new() should optionally run the same decoding on its message parameter.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dapi/src/services/platform_service/error_mapping.rs`:
- [SUGGESTION] lines 313-314: Pre-existing `is_ascii()` filter silently discards non-ASCII UTF-8 data
The `.filter(|s| s.is_ascii())` on line 313 (pre-existing, not introduced by this PR) means that if Tenderdash ever returns a valid UTF-8 `data` string containing non-ASCII characters (e.g. localized or user-provided error text), it gets silently dropped — `message` becomes `None` and later code falls back to a generic message instead of preserving the server detail.
`serde_json::Value::as_str()` already guarantees well-formed UTF-8, and `base64_decode()` will simply return `None` for non-base64 input, so the `is_ascii()` guard provides no safety benefit here. Removing it would make the "raw string preserved on decode failure" claim fully accurate.
Since this PR is already touching this exact code path, it would be a good opportunity to fix this.
- [SUGGESTION] lines 830-841: No integration test for valid-base64-but-not-CBOR data field
The existing `from_json_value_preserves_plain_text_data` test uses text that fails at the base64 step. Adding a test for a `data` field that base64-decodes successfully but is NOT valid CBOR would confirm the end-to-end fallback through the full `From<serde_json::Value>` conversion, not just the unit function.
In `packages/rs-dapi/src/services/platform_service/wait_for_state_transition_result.rs`:
- [SUGGESTION] lines 151-161: Other Tenderdash error paths still pass `data` through without CBOR decoding
The new `decode_data_message()` is only invoked inside `impl From<serde_json::Value>`, which handles JSON-RPC error responses. The `wait_for_state_transition_result` path constructs `TenderdashStatus::new()` directly from ABCI `tx_result.data` (lines 151–161, 243) without attempting CBOR decoding.
This may be intentional — the ABCI responses could use a different encoding format than JSON-RPC errors. But if they can also carry the same base64-CBOR payloads, clients would still receive unreadable strings on those paths. Worth investigating whether `TenderdashStatus::new()` should optionally run the same decoding on its `message` parameter.
packages/rs-dapi/src/services/platform_service/error_mapping.rs
Outdated
Show resolved
Hide resolved
- T4: Use non-logging base64 decode in decode_data_message() to avoid noisy debug logs when data field is plain text (expected failure) - T5: Fix misleading test comment — decode_data_message returns None, caller falls back to raw string - T6: Remove pre-existing .filter(|s| s.is_ascii()) that silently discarded valid non-ASCII UTF-8 data strings - T7: Add test for valid-base64-but-not-CBOR data field fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
added
the
ready for final review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Incremental Review
Single-commit push (25de8d2) addressing 2 of 3 prior suggestions. Changes are minimal, correct, and well-tested:
-
is_ascii()filter removed ✅ — resolves the pre-existing filter that silently dropped non-ASCII UTF-8 data strings. Thedecode_data_message()fallback now correctly handles all valid UTF-8. -
base64-non-CBOR integration test added ✅ — new
from_json_value_preserves_base64_non_cbor_dataconfirms end-to-end fallback for data that is valid base64 but not CBOR. -
Inline base64 engine — good call avoiding
base64_decode()debug logging for expected failures on plain-text data fields. The inlineGeneralPurposeengine keeps the same decode config.
All 36 tests pass (cargo test -p rs-dapi --lib error_mapping). No new findings.
Reviewed commit: 25de8d2
…-bin metadata When Drive returns a raw GroveDB storage error for a duplicate identity key (without a serialized ConsensusError), the SDK now checks the drive-error-data-bin gRPC metadata for CBOR-encoded error details. If the decoded message matches the identity key uniqueness pattern, the error is surfaced as Error::AlreadyExists instead of falling through to the opaque Error::DapiClientError. Also adds test fixtures for both the DAPI-level CBOR data field decoding and the SDK-level drive-error-data-bin handling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lklimek
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk/src/error.rs (1)
193-194: Make duplicate-key matching resilient to casing changes.Line 193 and Line 194 use case-sensitive
contains(...). If upstream wording changes capitalization, this mapping silently falls back toError::DapiClientError(_).♻️ Suggested patch
- if message.contains("unique key") && message.contains("already exists") { + let normalized = message.to_ascii_lowercase(); + if normalized.contains("unique key") + && normalized.contains("already exists") + { return Self::AlreadyExists(message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/error.rs` around lines 193 - 194, The duplicate-key detection is using case-sensitive contains on the error message, so capitalization changes cause falls back to DapiClientError; update the matching in the function that returns Self::AlreadyExists (the branch that currently does if message.contains("unique key") && message.contains("already exists")) to perform case-insensitive matching — e.g. normalize the message with message.to_lowercase() (or use a case-insensitive regex) and then check for "unique key" and "already exists", ensuring the same Self::AlreadyExists variant is returned when those substrings appear in any casing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-sdk/src/error.rs`:
- Around line 193-194: The duplicate-key detection is using case-sensitive
contains on the error message, so capitalization changes cause falls back to
DapiClientError; update the matching in the function that returns
Self::AlreadyExists (the branch that currently does if message.contains("unique
key") && message.contains("already exists")) to perform case-insensitive
matching — e.g. normalize the message with message.to_lowercase() (or use a
case-insensitive regex) and then check for "unique key" and "already exists",
ensuring the same Self::AlreadyExists variant is returned when those substrings
appear in any casing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 557e7cca-577e-488e-8ca0-2ba378c1e050
📒 Files selected for processing (2)
packages/rs-dapi/src/services/platform_service/error_mapping.rspackages/rs-sdk/src/error.rs
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "96246283cc6ebcde72308f6665fa756e3c8013a691f995e89692a97256b96a2b"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The new drive-error-data-bin detection code has a semantic conflict with existing recovery logic. When a duplicate identity key error (key belongs to a different identity) is mapped to Error::AlreadyExists, the Identity::wait_for_response() handler misinterprets it as 'this identity was already created', tries to fetch an identity that doesn't exist, and produces a misleading 'identity was proved to not exist but was said to exist' error. The helper function and tests are well-structured but this classification issue needs to be resolved before merging.
Reviewed commit: b18c173
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/error.rs`:
- [BLOCKING] lines 190-198: Mapping duplicate-key errors to AlreadyExists triggers wrong recovery path in Identity::wait_for_response
This new branch converts any `drive-error-data-bin` message containing 'unique key' and 'already exists' into `Error::AlreadyExists`. However, `Identity::wait_for_response()` in `waitable.rs:98-115` catches *every* `Error::AlreadyExists` and assumes the identity was previously created — it extracts the identity ID from the state transition and calls `Identity::fetch(sdk, identity_id)`. For a duplicate-key conflict the *key* belongs to a different identity, but the identity being created does not yet exist. The fetch returns `None` and the user receives the misleading error 'identity was proved to not exist but was said to exist', losing the actual duplicate-key cause entirely.
Either introduce a dedicated variant like `Error::DuplicateIdentityKey(String)` that is not caught by the existing `AlreadyExists` recovery path, or keep the error as `Error::DapiClientError` and let the caller inspect it. If `AlreadyExists` must be reused, update `waitable.rs` to distinguish between 'identity already exists' and 'key already exists' before attempting the fetch.
- [SUGGESTION] lines 193-195: Brittle string matching creates invisible coupling to Drive error format
The pattern `message.contains("unique key") && message.contains("already exists")` matches against Drive/GroveDB's human-readable error text (`UniqueKeyAlreadyExists` display string). If GroveDB or Drive ever changes the wording — even a minor phrasing tweak — this detection silently breaks with no compile-time or test-time signal. A more durable approach would be a machine-readable error code in the CBOR payload (the `code` field is already present but not used for this classification).
- [SUGGESTION] lines 190-192: Silent error swallowing when to_bytes() fails — inconsistent with adjacent code
When `drive_error_value.to_bytes()` returns `Err`, the failure is silently discarded via `if let Ok(bytes)`. The adjacent `consensus_error_value.to_bytes()` path at lines 181-185 logs at debug level and returns a `Generic` error. Adding a `tracing::debug!` log here would maintain consistency and aid debugging.
- [SUGGESTION] lines 190-198: Detection only covers UniqueKeyAlreadyExists, not IdentityKeyAlreadyExists
Drive defines two duplicate-key error variants: `UniqueKeyAlreadyExists` (display: 'a unique key with that hash already exists: ...') and `IdentityKeyAlreadyExists` (display: 'identity key already exists for user error: ...'). The string heuristic only matches the first variant. The second variant has different semantics (same-identity duplicate vs. cross-identity conflict) so this may be intentional, but it's worth documenting the distinction.
packages/rs-sdk/src/error.rs
Outdated
| #[test] | ||
| fn test_drive_error_data_bin_unrelated_message_falls_through() { | ||
| // When drive-error-data-bin contains a message that does NOT match | ||
| // the identity key pattern, the error should fall through to DapiClientError. | ||
| let mut cbor_buf = Vec::new(); | ||
| ciborium::ser::into_writer( | ||
| &ciborium::Value::Map(vec![( | ||
| ciborium::Value::Text("message".to_string()), | ||
| ciborium::Value::Text("some other storage error".to_string()), | ||
| )]), | ||
| &mut cbor_buf, | ||
| ) | ||
| .expect("CBOR serialize"); | ||
|
|
||
| let mut metadata = MetadataMap::new(); | ||
| metadata.insert_bin("drive-error-data-bin", MetadataValue::from_bytes(&cbor_buf)); | ||
|
|
||
| let status = | ||
| dapi_grpc::tonic::Status::with_metadata(Code::Internal, "Internal error", metadata); | ||
|
|
||
| let error = DapiClientError::Transport(TransportError::Grpc(status)); | ||
| let sdk_error = Error::from(error); | ||
|
|
||
| assert_matches!(sdk_error, Error::DapiClientError(_)); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: No test for malformed CBOR in drive-error-data-bin
The code handles invalid CBOR gracefully (ciborium::de::from_reader returns Err, inspect_err logs, .ok()? returns None → falls through). An explicit test with random bytes in drive-error-data-bin would document this behavior and prevent regressions.
source: ['claude']
…ror-data-bin metadata" This reverts commit b18c173.
…-bin metadata When a gRPC Internal status contains drive-error-data-bin metadata, extract the human-readable message from the CBOR payload and return Error::DriveInternalError instead of opaque Error::DapiClientError. This gives downstream consumers (like DET) a structured error variant for Drive-level failures that bypassed consensus validation. Related: #3396 (pre-validation gap for non-unique key set) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push correctly addresses the prior blocking issue by replacing the error-prone AlreadyExists mapping with a dedicated DriveInternalError variant, which no longer triggers the wrong recovery path in waitable.rs. The rs-dapi decoding logic and test coverage are solid. One gap: the SDK-side DriveInternalError conversion path and extract_drive_error_message() helper have no unit tests — the prior push's tests were removed along with the string-matching logic.
Reviewed commit: c27a959
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/error.rs`:
- [SUGGESTION] lines 194-210: SDK-side DriveInternalError conversion has no unit tests
The prior push had tests for the `drive-error-data-bin` handling path (`test_drive_error_data_bin_duplicate_identity_key` and `test_drive_error_data_bin_unrelated_message_falls_through`), but they were removed when the string-matching approach was replaced. The new `DriveInternalError` mapping and `extract_drive_error_message()` helper at lines 226-240 have zero test coverage in the SDK.
The rs-dapi side has thorough tests for `decode_data_message()`, but the SDK's `From<DapiClientError>` conversion through `drive-error-data-bin` metadata is a separate code path that deserves its own tests — particularly to verify that:
1. A gRPC `Internal` status with valid CBOR in `drive-error-data-bin` maps to `DriveInternalError`
2. A gRPC `Internal` status *without* `drive-error-data-bin` falls through to `DapiClientError`
3. Non-`Internal` status codes with `drive-error-data-bin` are NOT intercepted (the `Code::Internal` guard)
- [SUGGESTION] lines 198-204: Silent failure when to_bytes() fails on drive-error-data-bin metadata
When `drive_error_value.to_bytes()` returns `Err`, the failure is silently discarded via `if let Ok(bytes)`. The adjacent consensus error path (lines 181-190) handles `to_bytes()` failure by logging at debug level and returning a `Generic` error. Adding a `tracing::debug!` log here would maintain consistency and aid debugging — even a `trace!` level would be better than complete silence.
…ently discarding Addresses PR review feedback: the adjacent consensus error path logs at debug level on to_bytes() failure, but the drive-error-data-bin path silently swallowed the error. Now consistent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Improves error propagation across rs-dapi → gRPC metadata → rs-sdk by decoding Tenderdash data payloads that are base64-wrapped CBOR and surfacing Drive internal error messages as a dedicated SDK error variant.
Changes:
- Decode Tenderdash JSON-RPC
datafield as base64 → CBOR and extract a human-readablemessagewhen the top-level message is empty/“Internal error”. - Add
DriveInternalErrorto the SDK error enum and parsedrive-error-data-binmetadata onInternalgRPC responses to populate it. - Add unit tests in rs-dapi covering plain-text, base64-non-CBOR, and real-world fixtures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/rs-sdk/src/error.rs | Adds DriveInternalError and extracts message from CBOR drive-error-data-bin metadata for Internal gRPC statuses. |
| packages/rs-dapi/src/services/platform_service/error_mapping.rs | Decodes Tenderdash data base64 CBOR to recover readable messages and adds tests for this behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Drive returned an internal error that was not classified as a consensus | ||
| /// error. Contains the decoded human-readable message from the | ||
| /// `drive-error-data-bin` gRPC metadata (CBOR → message extraction). | ||
| /// | ||
| /// This typically indicates a storage-level error (e.g., GroveDB constraint | ||
| /// violation) that bypassed the consensus validation layer. If pre-validation | ||
| /// is working correctly, these should be rare. | ||
| #[error("Drive internal error: {0}")] | ||
| DriveInternalError(String), | ||
|
|
There was a problem hiding this comment.
Adding a new variant to the public Error enum (DriveInternalError) is a source-level breaking change for downstream crates that exhaustively match on dash_sdk::Error. If this crate is intended to preserve semver compatibility, consider marking the enum #[non_exhaustive] (and/or gating the new variant behind a feature) or updating release notes/versioning to reflect the breaking API change.
| // Check drive-error-data-bin for decoded Drive error messages | ||
| if status.code() == Code::Internal { | ||
| if let Some(drive_error_value) = status.metadata().get_bin("drive-error-data-bin") { | ||
| match drive_error_value.to_bytes() { | ||
| Ok(bytes) => { | ||
| if let Some(message) = extract_drive_error_message(&bytes) { | ||
| return Self::DriveInternalError(message); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| tracing::debug!( | ||
| "Failed to decode drive-error-data-bin metadata: {}", | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new drive-error-data-bin parsing branch isn’t covered by unit tests in this module (there are already tests for consensus metadata and AlreadyExists). Please add a test that constructs a tonic::Status with Code::Internal plus drive-error-data-bin containing CBOR { "message": "..." }, and assert it maps to Error::DriveInternalError with the decoded message.
| fn decode_data_message(data: &str) -> Option<String> { | ||
| // Silently try base64 — failure is expected for plain-text data fields, | ||
| // so we intentionally avoid `base64_decode()` which logs at debug level. | ||
| let decoded_bytes = engine::GeneralPurpose::new( | ||
| &base64::alphabet::STANDARD, | ||
| engine::GeneralPurposeConfig::new() | ||
| .with_decode_allow_trailing_bits(true) | ||
| .with_decode_padding_mode(engine::DecodePaddingMode::Indifferent), | ||
| ) | ||
| .decode(data) | ||
| .ok()?; | ||
|
|
There was a problem hiding this comment.
decode_data_message duplicates the base64 engine configuration that already exists in base64_decode(). To avoid config drift and make future changes safer, consider reusing the same engine (e.g., a shared static/helper) while still keeping this path silent (no debug logging) for expected non-base64 inputs.
| fn decode_data_message(data: &str) -> Option<String> { | |
| // Silently try base64 — failure is expected for plain-text data fields, | |
| // so we intentionally avoid `base64_decode()` which logs at debug level. | |
| let decoded_bytes = engine::GeneralPurpose::new( | |
| &base64::alphabet::STANDARD, | |
| engine::GeneralPurposeConfig::new() | |
| .with_decode_allow_trailing_bits(true) | |
| .with_decode_padding_mode(engine::DecodePaddingMode::Indifferent), | |
| ) | |
| .decode(data) | |
| .ok()?; | |
| /// Shared base64 engine configuration for Tenderdash error payloads. | |
| /// This mirrors the configuration used by other base64 helpers, so that | |
| /// future changes only need to be made in one place. | |
| fn tenderdash_base64_engine() -> engine::GeneralPurpose { | |
| engine::GeneralPurpose::new( | |
| &base64::alphabet::STANDARD, | |
| engine::GeneralPurposeConfig::new() | |
| .with_decode_allow_trailing_bits(true) | |
| .with_decode_padding_mode(engine::DecodePaddingMode::Indifferent), | |
| ) | |
| } | |
| fn decode_data_message(data: &str) -> Option<String> { | |
| // Silently try base64 — failure is expected for plain-text data fields, | |
| // so we intentionally avoid `base64_decode()` which logs at debug level. | |
| let decoded_bytes = tenderdash_base64_engine().decode(data).ok()?; |
Issue being fixed or feature implemented
When Tenderdash returns errors with
message: "Internal error", rs-dapi falls through to thedatafield. Tenderdash wraps Drive's CBOR error payload in the JSON-RPCdatafield as a base64 string, but rs-dapi stored it raw — producing unintelligible gRPC status messages likeoWdtZXNzYWdleMVzdG9yYWdl...on the wire.Related: dashpay/dash-evo-tool#766 (DET workaround using message-text matching)
Evidence from DET production logs
144 gRPC errors across all DET logs. Four patterns:
drive-error-data-bindash-serialized-consensus-error-binconsensus_errorin CBORThe majority of errors (99/144) are GroveDB duplicate-key errors during identity create/update where the actual human-readable message was buried in double-encoded CBOR. The 2 consensus errors (DPP 10530) already work correctly via the existing
dash-serialized-consensus-error-binpath.What was done?
Added
decode_data_message()helper that tries to decode thedatafield as base64 → CBOR → extract human-readablemessagetext. If decoding fails (plain text data), the raw string is preserved — no regression for existing behavior.Reuses existing
base64_decode()andwalk_cbor_for_key()helpers. 6 new tests including 2 real-world DET log fixtures.How Has This Been Tested?
Automated
cargo test -p rs-dapi --lib error_mapping— all 35 tests pass (29 existing + 6 new)cargo clippy -p rs-dapi— cleancargo fmt -p rs-dapi— cleanManual integration test
To reproduce with a real network:
IdentityCreatestate transitionInternalerror with messageoWdtZXNzYWdleMVzdG9yYWdl...(base64 CBOR gibberish)Internalerror with messagestorage: identity: a unique key with that hash already exists: the key already exists in the unique set [...]Alternatively, use the raw Tenderdash JSON-RPC fixture:
{ "code": 13, "message": "Internal error", "data": "oWdtZXNzYWdleMVzdG9yYWdlOiBpZGVudGl0eTogYSB1bmlxdWUga2V5IHdpdGggdGhhdCBoYXNoIGFscmVhZHkgZXhpc3RzOiB0aGUga2V5IGFscmVhZHkgZXhpc3RzIGluIHRoZSBub24gdW5pcXVlIHNldCBbMTM1LCAyMDIsIDE3MiwgNTMsIDE3NiwgNDUsIDE5MSwgMjcsIDUwLCAxMiwgNTAsIDIxNSwgNjUsIDEyNCwgMTQ3LCAzLCAyMDgsIDYsIDIyNiwgMTUxXQ==" }Breaking Changes
None. Plain text
datafields are preserved as-is. Only base64-encoded CBOR with amessagekey is decoded.Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
Tests