Fix: support 16kb page sizes #29
Conversation
WalkthroughAdds a GitHub Actions workflow that builds the project, verifies 16KB page alignment of 64-bit native libraries and AAR contents, and uploads artifacts. Updates toolchain and versions (JDK 17, AGP 8.5.1, Kotlin 2.1.20, NDK 29.x), increases SDK targets, bumps Rust crate versions, and applies small C++/SWIG fixes and .gitignore/local.properties changes. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as ubuntu-latest
participant Repo as Repository
participant Build as Gradle / CMake
participant NDK as Android NDK (llvm-readelf)
participant Store as Artifact upload
GH->>Runner: trigger on push/PR (master/main/develop)
Runner->>Repo: checkout (with submodules)
Runner->>Runner: setup JDK17, Android SDK/NDK, SWIG, cache Gradle
Runner->>Build: generate bindings, protobuf, cmake configure & build, gradle assemble
Build-->>Runner: produced AARs + native libs
Runner->>NDK: run llvm-readelf on 64-bit libs (inspect LOAD align)
NDK-->>Runner: alignment results per library
alt all LOAD align == 16384
Runner->>GH: report success (16KB alignment OK)
else any misaligned
Runner->>GH: fail job with misaligned libs + guidance
end
Runner->>Runner: scan AARs for `jni/` native libs
Runner->>Store: upload AARs, native libs, lint reports (7d retention)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
dash-sdk-java/src/main/swig/myexception.i (1)
7-17: Good improvement—considerconstreference.Catching exceptions by reference avoids unnecessary copies and prevents object slicing, which is C++ best practice. However, since the caught exceptions are not modified (only const methods like
c_str()andwhat()are called), consider usingconstreferences for correctness.Apply this diff to use const references:
- } catch(std::string& x) { + } catch(const std::string& x) { SWIG_exception(SWIG_JavaUnknownError, x.c_str()); - } catch(std::runtime_error& x) { + } catch(const std::runtime_error& x) { SWIG_exception(SWIG_RuntimeError, x.what()); - } catch(std::invalid_argument& x) { + } catch(const std::invalid_argument& x) { SWIG_exception(SWIG_TypeError, x.what()); - } catch(std::exception& x) { + } catch(const std::exception& x) { SWIG_exception(SWIG_UnknownError, x.what());dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt (1)
929-929: LGTM: Correct API update from deprecated method.The migration from
toLowerCase()tolowercase()correctly updates to the non-deprecated API and maintains existing behavior.For DPNS username normalization, consider using
lowercase(Locale.ROOT)orlowercase(Locale.US)to ensure consistent behavior across different system locales. This prevents issues like Turkish locale where 'I'.lowercase() becomes 'ı' instead of 'i', which could break username matching.Example:
- val normalizedName = username.lowercase() + val normalizedName = username.lowercase(Locale.ROOT)Note: This suggestion applies to all username normalization in this file (lines 929, 1680, 1671, 1736, 1742).
.github/workflows/build-and-verify-alignment.yml (1)
142-142: Minor: Add error handling for unzip operations in AAR verification.Lines 142 and 147 use
unzipwithout error checking. If a command fails (e.g., corrupted AAR file), the shell pipeline continues and returns a silent false negative (reports 0 libraries when unzip actually failed).Add error checking for unzip commands:
# Check if AAR contains native libraries - native_libs=$(unzip -l "$aar" | grep -E "jni/.*\.so$" | wc -l) + if ! unzip_output=$(unzip -l "$aar" 2>&1); then + echo " ❌ Error: Could not read AAR file" + continue + fi + native_libs=$(echo "$unzip_output" | grep -E "jni/.*\.so$" | wc -l) if [ $native_libs -gt 0 ]; then echo " ✅ Contains $native_libs native libraries" echo " 📋 Native library details:" - unzip -l "$aar" | grep -E "jni/.*\.so$" | awk '{print " " $4}' + echo "$unzip_output" | grep -E "jni/.*\.so$" | awk '{print " " $4}'Also applies to: 147-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/build-and-verify-alignment.yml(1 hunks).gitignore(1 hunks)build.gradle(2 hunks)dash-sdk-android/build.gradle(2 hunks)dash-sdk-android/src/main/rust/Cargo.toml(1 hunks)dash-sdk-bindings/Cargo.toml(1 hunks)dash-sdk-java/src/main/cpp/java_map_string_value.cpp(1 hunks)dash-sdk-java/src/main/swig/identity.i(1 hunks)dash-sdk-java/src/main/swig/myexception.i(1 hunks)dashj-platform-android-simple/build.gradle(2 hunks)dpp/build.gradle(1 hunks)dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt(3 hunks)dpp/src/main/java/org/dashj/platform/dpp/document/DocumentTransition.kt(1 hunks)local.properties(0 hunks)platform-mobile/Cargo.toml(1 hunks)
💤 Files with no reviewable changes (1)
- local.properties
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-verify
🔇 Additional comments (16)
.gitignore (1)
12-13: Good hygiene practices for build artifacts and local configuration.Both additions are appropriate for an Android/Kotlin project:
.DS_Storeprevents macOS system metadata files from being trackedlocal.propertiesprevents local Android build configuration (SDK paths, signing keys, etc.) from being accidentally committeddash-sdk-bindings/Cargo.toml (1)
3-3: Version bump aligns with coordinated release cycle.This change is part of the 2.0.2 version bump across related crates. No functional changes to dependencies or build configuration.
platform-mobile/Cargo.toml (1)
3-3: Version bump aligns with coordinated release cycle.This change is part of the 2.0.2 version bump across related crates. No functional changes to dependencies or build configuration.
dash-sdk-android/src/main/rust/Cargo.toml (1)
3-3: Version bump aligns with coordinated release cycle.This change is part of the 2.0.2 version bump across related crates. No functional changes to dependencies or build configuration.
dashj-platform-android-simple/build.gradle (1)
9-9: SDK, Java, and NDK versions updated appropriately for 16KB alignment.The updates to compileSdk 34, targetSdk 35, Java 17, and ndkVersion 29.0.14206865 are aligned with Android 15 requirements for 16KB page alignment. However, verify that Java 17 is compatible with all code in the project and with the updated Kotlin 2.1.20 compiler.
Also applies to: 13-13, 26-27, 34-34
dpp/build.gradle (1)
5-5: Detekt plugin version updated to 1.23.7.This is a routine maintenance update for the static analysis tool. No configuration changes required.
build.gradle (1)
3-3: Verify Kotlin 2.1.20 and AGP 8.5.1 compatibility.The Kotlin upgrade from 1.8.22 to 2.1.20 represents a major transition to the K2 compiler (Kotlin 2.0+), which introduced breaking changes and language modifications. Combined with AGP 8.5.1, comprehensive testing is required to confirm:
- All code compiles with the K2 compiler
- No deprecated or removed language features are in use
- Third-party dependencies are compatible with Kotlin 2.1.20
Also applies to: 13-13
dash-sdk-android/build.gradle (1)
14-14: NDK version update from 23.1 to 29.0 requires native build verification.The NDK version bump is significant (23.1→29.0) and necessary for 16KB page alignment support. Verify that:
- The CMake version (3.22.1 on line 41) is compatible with NDK 29.0.14206865
- All native code in
src/main/cppcompiles correctly with the new NDK- The native library build process completes without warnings or errors
The Java 17 and targetSdk 35 updates are appropriate for modern Android development.
Also applies to: 29-30, 37-37
dash-sdk-java/src/main/cpp/java_map_string_value.cpp (1)
5-5: LGTM! Correct include forstrdup().The addition of
#include <string.h>is necessary for thestrdup()call on line 50. This ensures proper compilation with modern compilers and stricter build settings.dash-sdk-java/src/main/swig/identity.i (1)
37-37: LGTM! Improved type consistency.Changing the loop index from
inttouintptr_taligns with pointer-sized indexing and matches the type ofcount, preventing potential sign/unsigned comparison warnings and ensuring correctness on all architectures.dpp/src/main/java/org/dashj/platform/dpp/document/DocumentTransition.kt (2)
26-28: LGTM: Correct API update from deprecated method.The migration from
toLowerCase()tolowercase()is correct and maintains the existing behavior.
30-32: LGTM: Correct API update from deprecated method.The migration from
toLowerCase()tolowercase()is correct and maintains the existing behavior.dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt (2)
1671-1671: LGTM: Correct API update from deprecated method.The migration from
toLowerCase()tolowercase()is correct and maintains existing behavior. The same optional locale consideration mentioned in the earlier comment applies here as well.Also applies to: 1680-1680
1736-1736: LGTM: Correct API update from deprecated method.The migration from
toLowerCase()tolowercase()is correct and maintains existing behavior. The same optional locale consideration mentioned earlier applies here as well.Also applies to: 1742-1742
.github/workflows/build-and-verify-alignment.yml (2)
1-42: LGTM on workflow setup and build configuration.Standard CI pipeline structure with appropriate JDK 17 and Android SDK setup.
154-171: LGTM on artifact upload configuration.Proper use of
if: always()to ensure artifacts are captured regardless of test result, and reasonable 7-day retention period.
| local all_aligned=true | ||
| for alignment in $alignments; do | ||
| # Convert hex to decimal | ||
| local decimal_alignment=$((alignment)) | ||
| local kb_alignment=$((decimal_alignment / 1024)) | ||
|
|
||
| echo " Segment alignment: $alignment ($decimal_alignment bytes = ${kb_alignment}KB)" | ||
|
|
||
| if [ $decimal_alignment -ne 16384 ]; then | ||
| echo " ❌ Not 16KB aligned (expected 0x4000 = 16384 bytes)" | ||
| all_aligned=false | ||
| else | ||
| echo " ✅ 16KB aligned" | ||
| fi | ||
| done | ||
|
|
||
| if [ "$all_aligned" = true ]; then | ||
| echo "✅ $lib_name is properly 16KB aligned" | ||
| return 0 | ||
| else | ||
| echo "❌ $lib_name has incorrect alignment" | ||
| return 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Critical: False positive if no LOAD segments found.
If readelf -l produces no output matching "LOAD" (line 65), the for loop at line 68 never executes, leaving all_aligned=true (initialized at line 67). The function then returns 0, incorrectly reporting success.
This creates a false positive if:
- readelf output format is unexpected
- grep "LOAD" matches nothing due to malformed library or parsing issue
- readelf itself fails silently
Apply this fix to detect missing or empty results:
# Check LOAD segments alignment
local alignments=$($readelf -l "$lib_path" | grep "LOAD" | awk '{print $NF}')
- local all_aligned=true
+ if [ -z "$alignments" ]; then
+ echo " ❌ No LOAD segments found (unable to verify alignment)"
+ return 1
+ fi
+
+ local all_aligned=true
for alignment in $alignments; doCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/build-and-verify-alignment.yml around lines 67 to 90: the loop
assumes there are LOAD segments and will report success when none are found;
detect empty or missing readelf results and treat that as a failure by checking
whether the alignments variable is empty (or readelf returned nothing) before
looping and set all_aligned=false / return non‑zero if so; implement an explicit
test after capturing alignments to bail out with an error message when no LOAD
entries were found (or when readelf returns non‑zero) so the job does not
produce a false positive.
| # Find all .so files in build outputs (64-bit architectures only) | ||
| echo "Finding 64-bit native libraries in build outputs..." | ||
| so_files=$(find . -path "*/build/intermediates/cxx/*/obj/*/*.so" -type f | grep -E "(Release|RelWithDebInfo)" | grep -E "(arm64-v8a|x86_64)" | head -20) | ||
|
|
||
| if [ -z "$so_files" ]; then | ||
| echo "❌ No native libraries found in build outputs" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Found native libraries:" | ||
| echo "$so_files" | ||
| echo |
There was a problem hiding this comment.
Major: Silent truncation with head -20 limits verification scope.
Line 94 silently truncates results to the first 20 native libraries. If a project has more than 20 native libraries, the remaining ones won't be checked, potentially missing alignment issues. The CI would pass despite incomplete verification.
Remove the head -20 limit to verify all libraries, or add a warning if truncation occurs:
- so_files=$(find . -path "*/build/intermediates/cxx/*/obj/*/*.so" -type f | grep -E "(Release|RelWithDebInfo)" | grep -E "(arm64-v8a|x86_64)" | head -20)
+ so_files=$(find . -path "*/build/intermediates/cxx/*/obj/*/*.so" -type f | grep -E "(Release|RelWithDebInfo)" | grep -E "(arm64-v8a|x86_64)")
+ library_count=$(echo "$so_files" | wc -l)
+ if [ "$library_count" -gt 50 ]; then
+ echo "⚠️ Found $library_count native libraries (checking all)"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Find all .so files in build outputs (64-bit architectures only) | |
| echo "Finding 64-bit native libraries in build outputs..." | |
| so_files=$(find . -path "*/build/intermediates/cxx/*/obj/*/*.so" -type f | grep -E "(Release|RelWithDebInfo)" | grep -E "(arm64-v8a|x86_64)" | head -20) | |
| if [ -z "$so_files" ]; then | |
| echo "❌ No native libraries found in build outputs" | |
| exit 1 | |
| fi | |
| echo "Found native libraries:" | |
| echo "$so_files" | |
| echo | |
| # Find all .so files in build outputs (64-bit architectures only) | |
| echo "Finding 64-bit native libraries in build outputs..." | |
| so_files=$(find . -path "*/build/intermediates/cxx/*/obj/*/*.so" -type f | grep -E "(Release|RelWithDebInfo)" | grep -E "(arm64-v8a|x86_64)") | |
| library_count=$(echo "$so_files" | wc -l) | |
| if [ "$library_count" -gt 50 ]; then | |
| echo "⚠️ Found $library_count native libraries (checking all)" | |
| fi | |
| if [ -z "$so_files" ]; then | |
| echo "❌ No native libraries found in build outputs" | |
| exit 1 | |
| fi | |
| echo "Found native libraries:" | |
| echo "$so_files" | |
| echo |
🤖 Prompt for AI Agents
.github/workflows/build-and-verify-alignment.yml around lines 92 to 103: the
script silently truncates the list of found .so files with `head -20`, which can
skip libraries and miss alignment issues; remove the `head -20` so `so_files`
contains all matching files, and add a simple count/warning after discovery
(e.g., print the number of files and a warning if it exceeds a threshold) so CI
either checks every library or explicitly warns when there are many results.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/build-and-verify-alignment.yml (2)
91-107:⚠️ Critical: False positive when no LOAD segments are found.If
readelf -l "$lib_path" | grep "LOAD"returns no output (e.g., due to unexpected readelf output format, truncated/corrupt library, or readelf failure), thealignmentsvariable will be empty. Theforloop at line 94 will never execute, leavingall_aligned=true, and the function returns 0 (line 111), incorrectly reporting success.Detect and reject missing or empty results before entering the loop:
# Check LOAD segments alignment local alignments=$($readelf -l "$lib_path" | grep "LOAD" | awk '{print $NF}') + if [ -z "$alignments" ]; then + echo " ❌ No LOAD segments found in $lib_name (unable to verify alignment)" + return 1 + fi + local all_aligned=true for alignment in $alignments; do
120-120:⚠️ Major: Silent truncation withhead -20skips libraries.Line 120 silently limits verification to the first 20 native libraries. If the project builds more than 20 native libraries, the remaining ones are not checked, allowing misaligned libraries to slip through CI undetected.
Remove the
head -20limit to verify all libraries, or add explicit logging of the count:- so_files=$(find . -path "*/build/intermediates/cxx/*/obj/*/*.so" -type f | grep -E "(Release|RelWithDebInfo)" | grep -E "(arm64-v8a|x86_64)" | head -20) + so_files=$(find . -path "*/build/intermediates/cxx/*/obj/*/*.so" -type f | grep -E "(Release|RelWithDebInfo)" | grep -E "(arm64-v8a|x86_64)") + library_count=$(echo "$so_files" | wc -l) + echo "Found $library_count total 64-bit native libraries"
🧹 Nitpick comments (1)
.github/workflows/build-and-verify-alignment.yml (1)
164-178: Minor: Improve robustness of AAR path and output handling.The loop at lines 164–178 processes AAR file paths. If file paths contain special characters or spaces, the loop iteration could break. Additionally,
unzipshould be available onubuntu-latest, but it's not explicitly required. Consider adding a guard or comment:echo "Found AAR files:" for aar in $aar_files; do - echo "📦 $(basename $aar)" + aar_name=$(basename "$aar") + echo "📦 $aar_name"This is a minor robustness improvement; current behavior is likely safe for typical AAR naming conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-and-verify-alignment.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/build-and-verify-alignment.yml (2)
28-32: ✅ NDK installation now properly configured.You've added an explicit NDK installation step with
sdkmanager, which correctly setsANDROID_NDK_ROOTas an environment variable. This resolves the previous blocker where the alignment verification script couldn't locatellvm-readelf. The NDK version29.0.14206865matches the expectations downstream.
75-88: Verify llvm-readelf availability is properly handled.The check at lines 85–88 exits early if
llvm-readelfis not found. However, the error message will only appear in logs; ensure the workflow treats this as a hard failure (the function does return 1, which is correct). As a secondary check, verify that the NDK installation path (set at line 32) matches the expected layout at line 83.
Summary by CodeRabbit
New Features
Chores
Bug Fixes