Use direct partition API for share storage#9
Conversation
|
Warning Rate limit exceeded@kwsantiago has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (27)
WalkthroughThis PR removes libnostr-c/noscrypt component wiring, replaces NVS storage with a partition-backed slot storage, switches USB/ROM serial I/O to the UART driver with line buffering, restructures the main RPC loop and build deps, and adds new serial/RPC test scripts and sdkconfig defaults. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Host as Host (test_all_rpc.py / user)
participant Serial as UART
participant Firmware as ESP32-S3 Firmware
participant Storage as Partition
participant Crypto as FROST signer
Host->>Serial: open /dev/ttyUSB0, send JSON-RPC request
Serial->>Firmware: UART RX delivers full line
Firmware->>Firmware: parse JSON request (line-based)
alt storage-related
Firmware->>Storage: read/scan slots (list/load/delete/save)
Storage-->>Firmware: slot data / ack
end
alt signing flow
Firmware->>Crypto: frost_commit / frost_sign operations
Crypto-->>Firmware: commitment / signature or error
end
Firmware->>Serial: format JSON-RPC response line
Serial->>Host: host reads response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
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: 11
🧹 Nitpick comments (4)
scripts/test_ping.py (1)
8-8: Remove unnecessary f-string prefix.The string has no placeholders, so the
fprefix is unnecessary.🔎 Proposed fix
- print(f'Sending ping...') + print('Sending ping...')scripts/monitor_serial.py (1)
13-13: Consider reading in larger chunks for efficiency.Reading one byte at a time can be inefficient. Consider reading in larger chunks (e.g., 1024 bytes) to reduce system call overhead while maintaining real-time display.
main/serial.c (1)
19-31: Simplify redundant newline stripping.The code strips trailing newline/carriage return characters twice (lines 23-26, then 27-30). While defensive, this is redundant since a string can only have one trailing newline sequence. Consider simplifying.
🔎 Proposed fix
int serial_read_line(char *buf, size_t len) { if (fgets(buf, len, stdin) == NULL) { return 0; } size_t l = strlen(buf); - if (l > 0 && (buf[l-1] == '\n' || buf[l-1] == '\r')) { - buf[l-1] = '\0'; - l--; - } - if (l > 0 && (buf[l-1] == '\n' || buf[l-1] == '\r')) { + // Strip trailing \r\n or \n\r sequences + while (l > 0 && (buf[l-1] == '\n' || buf[l-1] == '\r')) { buf[l-1] = '\0'; l--; } return (int)l; }main/storage.c (1)
46-58: Consider handling uninitialized/corrupted partition data.If the partition contains garbage data after first flash, slots with random
share_lenvalues could be misinterpreted as valid. Consider erasing the entire partition on first init or adding a magic number toshare_slot_tto detect uninitialized slots.🔎 Proposed enhancement
Add a magic number to the structure:
+#define SLOT_MAGIC 0x53484152 // "SHAR" + typedef struct { + uint32_t magic; uint8_t group_hash[32]; uint16_t share_len; uint8_t share_data[STORAGE_SHARE_LEN]; - uint8_t reserved[226]; + uint8_t reserved[222]; // Adjust for magic field } __attribute__((packed)) share_slot_t;Then check
slot.magic == SLOT_MAGICwhen determining if a slot is valid.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
partitions.csvis excluded by!**/*.csv
📒 Files selected for processing (24)
README.mdcomponents/libnostr-c/CMakeLists.txtcomponents/libnostr-c/idf_component.ymlcomponents/libnostr-c/include/cjson/cJSON.hcomponents/libnostr-c/include/noscrypt/noscrypt.hcomponents/libnostr-c/include/noscrypt/noscryptutil.hcomponents/libnostr-c/nostr_features.hcomponents/noscrypt/CMakeLists.txtcomponents/noscrypt/idf_component.ymlcomponents/noscrypt/include/noscrypt/noscrypt.hcomponents/noscrypt/include/noscrypt/noscryptutil.hcomponents/noscrypt/include/noscrypt/platform.hcomponents/noscrypt/include/secp256k1/include/secp256k1.hcomponents/noscrypt/include/secp256k1/include/secp256k1_ecdh.hcomponents/noscrypt/include/secp256k1/include/secp256k1_extrakeys.hcomponents/noscrypt/include/secp256k1/include/secp256k1_schnorrsig.hmain/CMakeLists.txtmain/main.cmain/serial.cmain/storage.cscripts/monitor_serial.pyscripts/test_ping.pyscripts/test_rpc.pysdkconfig.defaults
💤 Files with no reviewable changes (15)
- components/libnostr-c/include/noscrypt/noscryptutil.h
- components/libnostr-c/include/cjson/cJSON.h
- components/noscrypt/include/noscrypt/noscrypt.h
- components/noscrypt/include/secp256k1/include/secp256k1_ecdh.h
- components/noscrypt/CMakeLists.txt
- components/noscrypt/include/noscrypt/platform.h
- components/noscrypt/idf_component.yml
- components/noscrypt/include/secp256k1/include/secp256k1_extrakeys.h
- components/noscrypt/include/secp256k1/include/secp256k1.h
- components/libnostr-c/include/noscrypt/noscrypt.h
- components/noscrypt/include/secp256k1/include/secp256k1_schnorrsig.h
- components/libnostr-c/nostr_features.h
- components/libnostr-c/CMakeLists.txt
- components/noscrypt/include/noscrypt/noscryptutil.h
- components/libnostr-c/idf_component.yml
🧰 Additional context used
🧬 Code graph analysis (1)
main/main.c (3)
main/frost_signer.c (1)
frost_signer_init(108-114)main/serial.c (3)
serial_init(14-16)serial_read_line(18-32)serial_write_line(34-38)main/protocol.c (2)
protocol_parse_request(17-64)protocol_format_response(66-93)
🪛 Ruff (0.14.10)
scripts/test_ping.py
8-8: f-string without any placeholders
Remove extraneous f prefix
(F541)
25-25: Do not use bare except
(E722)
25-26: try-except-pass detected, consider logging the exception
(S110)
scripts/test_rpc.py
23-23: Do not use bare except
(E722)
scripts/monitor_serial.py
19-19: Do not catch blind exception: Exception
(BLE001)
22-22: Do not catch blind exception: Exception
(BLE001)
⏰ 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
🔇 Additional comments (5)
main/CMakeLists.txt (1)
4-4: LGTM! Dependencies align with the storage refactor.The addition of
esp_partitionandmbedtlsdependencies supports the new partition-based storage implementation, while removingnvs_flashis consistent with eliminating NVS usage.sdkconfig.defaults (2)
5-5: Consider the implications of disabling the task watchdog.Disabling
CONFIG_ESP_TASK_WDT_ENremoves automatic detection of hung tasks. While this may be acceptable for development or if the application has its own monitoring, it could mask issues in production deployments.
2-3: No action needed—partitions.csv exists in the repository and is properly tracked by git.The custom partition table file referenced in the configuration is present at the repository root.
Likely an incorrect or invalid review comment.
README.md (1)
9-9: LGTM! Documentation accurately reflects the implementation changes.The documentation updates correctly describe:
- The migration from NVS to partition-backed storage
- Hardware requirements for ESP32-S3
- Updated device paths for USB Serial JTAG (/dev/ttyACM0)
- New test scripts and monitoring tools
These changes align well with the code refactoring throughout the PR.
Also applies to: 24-28, 33-40, 48-51, 57-82
main/main.c (1)
138-138: Good addition of task delay.The 10ms delay prevents the main loop from spinning tightly and consuming excessive CPU, allowing other FreeRTOS tasks to run.
kwsantiago
changed the title
kwsantiago
force-pushed
the
fix/nvs-memory-exhaustion
branch
from
d6ae984 to
c7f4fff
Compare
kwsantiago
changed the title
kwsantiago
force-pushed
the
fix/nvs-memory-exhaustion
branch
from
c7f4fff to
dcd4ba1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
sdkconfig.defaults (1)
12-12: Consider re-enabling the task watchdog for production.Disabling
CONFIG_ESP_TASK_WDT_ENis useful during development/debugging, but in production it removes protection against infinite loops or stuck tasks. Consider usingCONFIG_ESP_TASK_WDT_PANIC=ninstead to allow watchdog monitoring without immediate panic, or add a comment indicating this is intentional for testing.main/CMakeLists.txt (1)
4-4: Dependencies updated correctly for partition-backed storage.Adding
esp_partitionandmbedtlswhile removingnvs_flashaligns with the storage refactor.Consider removing
esp_driver_usb_serial_jtagif it's no longer used after switching to UART-based serial—this would reduce unnecessary dependencies.main/storage.c (1)
59-64: Consider validating input parameters before processing.The function doesn't validate that
groupandshare_hexare non-NULL or thatgrouplength fits withinSTORAGE_GROUP_LEN. While callers may validate inputs, defensive checks here would prevent crashes from unexpected NULL pointers.🔎 Proposed defensive checks
int storage_save_share(const char *group, const char *share_hex) { if (!initialized) return -1; + if (!group || !share_hex || strlen(group) == 0) return -1; + if (strlen(group) > STORAGE_GROUP_LEN) return -1; unsigned char share_bytes[STORAGE_SHARE_LEN]; int share_len = hex_to_bytes(share_hex, share_bytes, sizeof(share_bytes));scripts/test_all_rpc.py (2)
79-79: Remove extraneous f-string prefixes.Lines 79 and 97 use f-strings without any placeholders. Per static analysis (Ruff F541).
🔎 Proposed fix
if resp and "result" in resp: - print(f" PASS: commitment received") + print(" PASS: commitment received") return resp['result']if resp and "result" in resp: - print(f" PASS: signature share received") + print(" PASS: signature share received") return TrueAlso applies to: 97-97
10-30: Consider consolidating shared RPC utilities withtests/test_rpc.py.Both
scripts/test_all_rpc.pyandtests/test_rpc.pyimplement similarsend_request/send_rpcfunctions with slight differences (line endings\r\nvs\n, timeout handling). Consider extracting common utilities to a shared module to reduce duplication and ensure consistent behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
partitions.csvis excluded by!**/*.csv
📒 Files selected for processing (25)
README.mdcomponents/libnostr-c/CMakeLists.txtcomponents/libnostr-c/idf_component.ymlcomponents/libnostr-c/include/cjson/cJSON.hcomponents/libnostr-c/include/noscrypt/noscrypt.hcomponents/libnostr-c/include/noscrypt/noscryptutil.hcomponents/libnostr-c/nostr_features.hcomponents/noscrypt/CMakeLists.txtcomponents/noscrypt/idf_component.ymlcomponents/noscrypt/include/noscrypt/noscrypt.hcomponents/noscrypt/include/noscrypt/noscryptutil.hcomponents/noscrypt/include/noscrypt/platform.hcomponents/noscrypt/include/secp256k1/include/secp256k1.hcomponents/noscrypt/include/secp256k1/include/secp256k1_ecdh.hcomponents/noscrypt/include/secp256k1/include/secp256k1_extrakeys.hcomponents/noscrypt/include/secp256k1/include/secp256k1_schnorrsig.hmain/CMakeLists.txtmain/main.cmain/serial.cmain/storage.cscripts/monitor_serial.pyscripts/test_all_rpc.pysdkconfig.defaultstests/run_qemu_test.shtests/test_rpc.py
💤 Files with no reviewable changes (16)
- components/noscrypt/include/noscrypt/noscrypt.h
- components/libnostr-c/include/cjson/cJSON.h
- components/noscrypt/include/noscrypt/platform.h
- components/noscrypt/include/secp256k1/include/secp256k1.h
- components/noscrypt/include/noscrypt/noscryptutil.h
- components/noscrypt/include/secp256k1/include/secp256k1_ecdh.h
- components/libnostr-c/idf_component.yml
- components/libnostr-c/CMakeLists.txt
- components/noscrypt/CMakeLists.txt
- components/noscrypt/include/secp256k1/include/secp256k1_schnorrsig.h
- components/libnostr-c/include/noscrypt/noscryptutil.h
- tests/run_qemu_test.sh
- components/noscrypt/include/secp256k1/include/secp256k1_extrakeys.h
- components/libnostr-c/include/noscrypt/noscrypt.h
- components/noscrypt/idf_component.yml
- components/libnostr-c/nostr_features.h
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/monitor_serial.py
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/test_all_rpc.py (1)
tests/test_rpc.py (6)
test_ping(20-25)test_import_share(35-43)test_frost_commit(56-69)test_frost_sign(71-84)test_delete_share(86-90)main(95-140)
🪛 Ruff (0.14.10)
scripts/test_all_rpc.py
79-79: f-string without any placeholders
Remove extraneous f prefix
(F541)
97-97: f-string without any placeholders
Remove extraneous f prefix
(F541)
131-131: Do not catch blind exception: Exception
(BLE001)
⏰ 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
🔇 Additional comments (12)
tests/test_rpc.py (1)
96-96: LGTM!The default port change from
/dev/ttyACM0to/dev/ttyUSB0aligns with the shift from USB Serial JTAG to UART-based serial communication configured insdkconfig.defaults.README.md (2)
9-9: Documentation improvements look good.The updated storage description, new hardware section, and expanded testing documentation provide clear guidance for users. The testing section now properly references the new RPC test scripts.
Also applies to: 24-28, 68-78
80-81: Thetests/test_qemu.pyfile exists in the repository and is correctly referenced in the documentation. No issues to address.main/main.c (2)
108-112: Previous review concerns addressed, but continuing after storage failure may cause issues.The error response for parse failures (lines 128-130) and format response check (lines 131-136) address past review comments. However, continuing after
storage_init()failure (line 109) means all storage-dependent RPC methods (import_share,delete_share,list_shares,get_share_pubkey) will fail with storage errors. Consider whether this degraded mode is intentional or if it would be better to halt initialization.
117-138: Main loop implementation is clean and handles errors appropriately.The static allocation for buffers avoids heap fragmentation on the embedded device. The loop properly:
- Resets response state each iteration
- Sends error responses for parse failures
- Checks format response return value before writing
- Yields to allow other FreeRTOS tasks to run
main/serial.c (1)
20-36: Line reading implementation is correct.The buffered input approach properly handles:
- CR/LF line terminators
- Buffer overflow protection (line 31)
- Null-termination of output
One minor note: if the caller's buffer is smaller than the accumulated data, the line is silently truncated. This is acceptable given the matching buffer sizes used in
main.c.main/storage.c (3)
13-18: Previous critical issues resolved.The struct sizing issue is now fixed with
_Static_assertat line 153 ensuringshare_slot_tequalsSHARE_SLOT_SIZE(512 bytes). The struct now stores the group as a string instead of a hash, simplifying lookups and addressing the previous concern about needing to store original group strings.Also applies to: 153-153
81-98: Partition erase alignment issue resolved.The sector-based read-modify-write approach now properly aligns erase operations to 4KB boundaries (
SECTOR_SIZE). The implementation:
- Reads the entire 4KB sector containing the target slot
- Modifies the specific slot within the buffer
- Erases the aligned sector
- Writes the complete sector back
This correctly addresses the previous critical issue about ESP32's 4KB flash sector alignment requirement.
155-170:storage_list_sharesis now properly implemented.The function correctly:
- Checks initialization state (returns -1 if uninitialized)
- Iterates slots to find valid entries
- Copies group strings with proper bounds
- Returns the count of groups found
This addresses the previous concern about the stub implementation.
scripts/test_all_rpc.py (2)
127-133: Broad exception handling is acceptable here.While Ruff flags
BLE001for catching blindException, this is appropriate for serial port initialization where various OS-level errors can occur (permission denied, device not found, etc.). The error is logged with context before exiting.
84-103: Test logic appropriately handles expected errors.The
test_frost_signfunction correctly treats an error response as a pass (lines 99-101) since invalid commitments are expected to fail. This is appropriate for testing error handling paths.sdkconfig.defaults (1)
1-15: Serial driver missing explicit baud rate configuration.While the configuration correctly enables UART0, the claim that it "correctly matches the serial driver configuration" is inaccurate. The
serial.cimplementation callsuart_driver_install()without a precedinguart_param_config()call to set the baud rate. TheCONFIG_ESP_CONSOLE_UART_BAUDRATE=115200in sdkconfig.defaults applies only to the ESP console subsystem, not to the custom serial driver. The serial driver will use UART defaults rather than explicitly configured 115200 baud.The
partitions.csvfile correctly exists and defines the required "storage" partition. The stack size and partition table settings are appropriate, but the serial driver implementation needs a call touart_param_config()to explicitly configure the UART parameters before use.Likely an incorrect or invalid review comment.
kwsantiago
force-pushed
the
fix/nvs-memory-exhaustion
branch
from
dcd4ba1 to
e6bb882
Compare
kwsantiago
force-pushed
the
fix/nvs-memory-exhaustion
branch
from
e6bb882 to
2b67778
Compare
1 participant
Closes #8
Summary by CodeRabbit
Documentation
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.