Repository navigation
Conversation
UnsignedByte is mutable (destroy() zeroes it), but UnsignedByteArray stored caller-supplied instances by reference. PrivateKey.prefixedBytes() and BcKeyUtils.toPublicKey() built arrays whose first element was the shared static ED2559_PREFIX / SECP256K1_PREFIX constant, so destroying an exported key zeroed the process-wide prefix: later ed25519 exports carried a 0x00 prefix and fromPrefixedBytes misclassified keys. IssueType shared the static ACCOUNT_ONE bytes the same way, and slice() returned a subList view whose destroy() wiped the parent. UnsignedByteArray now copies every UnsignedByte it receives via the public constructor, of(UnsignedByte...), append(UnsignedByte), append(UnsignedByteArray) and set(), and returns independent arrays from slice() and reverse(). Factories that already allocate fresh instances use a private no-copy constructor so of(byte[]), fromHex, ofSize and empty are unchanged in cost. Adds aliasing tests for each UnsignedByteArray path and regression tests for PrivateKey, BcKeyUtils, Seed and IssueType, plus a test helper that restores the shared constants so a regression cannot poison the fork. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Copy elements across array operations xrpl4j-core/src/main/java/org/xrpl/xrpl4j/codec/addresses/UnsignedByteArray.java, xrpl4j-core/src/test/java/org/xrpl/xrpl4j/codec/addresses/UnsignedByteArrayTest.java |
Construction, factory, append, set, slice, and reverse operations now use independent UnsignedByte elements. Tests check that destroying a result leaves its inputs and source arrays unchanged. |
Serialization and key-prefix regression tests xrpl4j-core/src/test/java/org/xrpl/xrpl4j/codec/addresses/UnsignedByteTestUtils.java, xrpl4j-core/src/test/java/org/xrpl/xrpl4j/codec/binary/types/IssueTypeTest.java, xrpl4j-core/src/test/java/org/xrpl/xrpl4j/crypto/keys/PrivateKeyPrefixDestroyTest.java, xrpl4j-core/src/test/java/org/xrpl/xrpl4j/crypto/keys/SeedTest.java, xrpl4j-core/src/test/java/org/xrpl/xrpl4j/crypto/keys/bc/BcKeyUtilsTest.java |
Tests check that destroying serialized or key values does not alter shared prefixes or later serialization and conversion results. A test utility resets shared UnsignedByte values. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Merge Risk: 
4ed8a
-
The ownership changes satisfy the intended isolation behavior, with no established merge-blocking risk.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely identifies the main change: defensive copying of UnsignedByte instances in UnsignedByteArray. Description check The description directly explains the aliasing defect, the defensive-copying fix, affected call sites, API behavior, and regression tests. Docstring Coverage Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The private non-copying constructor of UnsignedByteArray used an unused `boolean ignored` parameter purely to disambiguate it from the public copying constructor. Replace it with a private `Ownership` enum so call sites read as `new UnsignedByteArray(list, Ownership.TAKE)`, making the take-ownership intent explicit and silencing the unused-parameter warning. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UnsignedByteis mutable (destroy()zeroes its value), butUnsignedByteArraystored caller-suppliedUnsignedByteinstances by reference. Several call sites build arrays from shared static constants, so destroying one array mutated process-wide state:PrivateKey.prefixedBytes()placed the sharedPrivateKey.ED2559_PREFIX/SECP256K1_PREFIXconstant at index 0. Callingdestroy()on the exported array setED2559_PREFIXto0x00for the whole JVM. After that, fresh ed25519 exports carried a00prefix,fromPrefixedBytesrejected genuineED-prefixed keys, and00-prefixed input was classified as ED25519 because both constants compared equal.BcKeyUtils.toPublicKey(Ed25519PublicKeyParameters)storedPublicKey.ED2559_PREFIXinside the resultingPublicKey.value(), with the same exposure.IssueTypeappended the staticACCOUNT_ONEarray into every MPT issue serialization viaappend(UnsignedByteArray), which usedaddAllwithout copying.slice()returned asubListview, so destroying a slice wiped the parent array.Fix
Fixed at the
UnsignedByteArraylevel so the class never aliases anUnsignedByteit did not allocate:of(UnsignedByte...),append(UnsignedByte),append(UnsignedByteArray)andset()copy their inputs.slice()andreverse()return independent arrays.of(byte[]),fromHex,ofSize,empty) use a private no-copy constructor, so their cost is unchanged.appendkeeps its documented mutate-and-return-this chaining semantics.Note:
append(UnsignedByteArray)now allocates oneUnsignedByteper appended byte on the codec path where it previously shared references.Tests
Written red-first; 14 failures before the fix, all green after.
UnsignedByteArrayTest: one aliasing test per path (constructor,of, bothappendoverloads,set,slice,reverse).PrivateKeyPrefixDestroyTest(new): destroyingprefixedBytes()leaves both prefix constants intact andfromPrefixedBytesstill classifies correctly.BcKeyUtilsTest:toPublicKeydoes not sharePublicKey.ED2559_PREFIX; destroying its value leaves the constant intact.IssueTypeTest: destroying an MPT issue's bytes does not change the next serialization.SeedTest: derived ed25519 public key does not share the prefix.UnsignedByteTestUtils(new, test-only): reflection reset of shared constants, used as a safety net so a future regression cannot poison the rest of the fork.Full
xrpl4j-coreunit suite and checkstyle pass.🤖 Generated with Claude Code