wallet: restrict sensitive RPC commands under --restricted-rpc - #212
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe wallet RPC minor version increases to 18, and five commands become restricted. Protocol handlers reject oversized messages. Release builds use stronger fortification flags. Debian/Ubuntu instructions now install the libcurl development package. ChangesWallet RPC access modes
Protocol request validation
Build and setup updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/wallet_rpc_server_commands_defs.h`:
- Line 1368: Update WALLET_RPC_VERSION_MINOR from 17 to 18 in the wallet RPC
version definitions to reflect the access-control changes in EXPORT_TRANSFERS
and the other RESTRICTED command declarations at the referenced locations. Keep
the existing major version unless the project’s versioning rules classify this
restriction as incompatible.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 316c94ea-49e8-4668-9e2f-6bfbb0029755
📒 Files selected for processing (1)
src/wallet/wallet_rpc_server_commands_defs.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/wallet_rpc_server_commands_defs.h`:
- Line 1671: Update WALLET_RPC_VERSION_MINOR from 17 to 18 in the wallet RPC
version definitions to reflect the restricted-mode availability changes in
START_MINING and the other affected command definition; use the major-version
rule only if this change is classified as incompatible.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a9ec2c19-9fb2-4c65-a497-6873ac3c9c95
📒 Files selected for processing (1)
src/wallet/wallet_rpc_server_commands_defs.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 70: Update the Debian/Ubuntu dependency entry in the README to list the
required libcurl development package, libcurl4-openssl-dev, instead of relying
on the curl package; document both only if the runtime curl package is also
required.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f7a962f-7621-4f81-a006-ead8af9f50ce
📒 Files selected for processing (2)
CMakeLists.txtREADME.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cryptonote_protocol/cryptonote_protocol_handler.inl`:
- Around line 1851-1858: Enforce CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT
while deserializing NOTIFY_REQUEST_CHAIN::request, before buff_to_t_adapter
resizes or inserts entries into block_ids. Reject oversized serialized lists
immediately and preserve handle_request_chain’s existing validation for
already-deserialized requests.
- Around line 2525-2531: Update the producer path that calls get_mined_flashes()
and posts NOTIFY_RESPONSE_BLOCK_FLASHES to cap each response at
CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT (5000) or split larger results into
multiple responses. Ensure every matching flash hash is delivered through
bounded batches so the receiver’s validation in cryptonote_protocol_handler does
not drop the connection.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c0ad6b7d-bf08-4ffa-ae21-c1a1181eefee
📒 Files selected for processing (1)
src/cryptonote_protocol/cryptonote_protocol_handler.inl
Comment on lines
+1851
to
+1858
|
|
||
| if (arg.block_ids.size() > CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT) | ||
| { | ||
| LOG_ERROR_CCONTEXT("Too many block IDs requested: " << arg.block_ids.size() << " (maximum " << CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT << ")"); | ||
| drop_connection(context, false, false); | ||
| return 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'HANDLE_NOTIFY_T2|load_t_from_binary|NOTIFY_REQUEST_CHAIN|MAX_PACKET|MAX.*MESSAGE' \
--glob '*.h' --glob '*.hpp' --glob '*.cpp' --glob '*.inl' . || trueRepository: Beldex-Coin/beldex
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- container serialization macros ---'
rg -n -C 8 'KV_SERIALIZE_CONTAINER_POD_AS_BLOB|serialize_container|load_from_binary|load_storage' \
contrib/epee/include/epee src/cryptonote_protocol \
--glob '*.h' --glob '*.hpp' --glob '*.cpp' --glob '*.inl' \
| head -n 500
printf '%s\n' '--- P2P deserialization call path ---'
rg -n -C 10 'buff_to_t_adapter|load_t_from_binary|handle_invoke_map|in_buff' \
contrib/epee/include/epee/net contrib/epee/include/epee/storages \
--glob '*.h' --glob '*.hpp' --glob '*.cpp' --glob '*.inl' \
| head -n 500Repository: Beldex-Coin/beldex
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- binary reader array and packet bounds ---'
sed -n '40,145p' contrib/epee/include/epee/storages/portable_storage_from_bin.h
sed -n '215,250p' contrib/epee/include/epee/storages/portable_storage_from_bin.h
sed -n '130,170p' contrib/epee/include/epee/net/levin_protocol_handler.h
sed -n '155,220p' contrib/epee/include/epee/storages/levin_abstract_invoke2.h
printf '%s\n' '--- blob-container deserialization ---'
rg -n -C 12 'perform_serialize_blob_container|serialize_blob_container|blob_container' \
contrib/epee/include/epee/serialization \
--glob '*.h' --glob '*.hpp' --glob '*.cpp'Repository: Beldex-Coin/beldex
Length of output: 14352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '180,290p' contrib/epee/include/epee/serialization/keyvalue_serialization_overloads.h
rg -n -C 15 'unserialize_stl_container_pod_val_as_blob|serialize_stl_container_pod_val_as_blob|MAX_STRING_LEN_POSSIBLE' \
contrib/epee/include/epee \
--glob '*.h' --glob '*.hpp' --glob '*.cpp'Repository: Beldex-Coin/beldex
Length of output: 24708
Reject oversized block_ids during deserialization.
buff_to_t_adapter loads the portable storage and populates NOTIFY_REQUEST_CHAIN::request before handle_request_chain runs. The blob deserializer has no CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT check and can populate far more than 500 list entries before the guard executes. Enforce the limit before resizing or inserting into the container.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cryptonote_protocol/cryptonote_protocol_handler.inl` around lines 1851 -
1858, Enforce CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT while deserializing
NOTIFY_REQUEST_CHAIN::request, before buff_to_t_adapter resizes or inserts
entries into block_ids. Reject oversized serialized lists immediately and
preserve handle_request_chain’s existing validation for already-deserialized
requests.
Comment on lines
+2525
to
+2531
| if (arg.txs.size() > CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT) | ||
| { | ||
| LOG_ERROR_CCONTEXT("Too many flash transactions received: " << arg.txs.size() << " (maximum " << CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT << ")"); | ||
| drop_connection(context, false, false); | ||
| return 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'handle_request_block_flashes|get_mined_flashes|NOTIFY_RESPONSE_BLOCK_FLASHES|CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT' \
--glob '*.h' --glob '*.hpp' --glob '*.cpp' --glob '*.inl' . || trueRepository: Beldex-Coin/beldex
Length of output: 31334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- handler and producer implementation ---'
sed -n '2488,2555p' src/cryptonote_protocol/cryptonote_protocol_handler.inl
sed -n '630,680p' src/cryptonote_core/tx_pool.cpp
printf '%s\n' '--- all flash request construction and dispatch sites ---'
rg -n -C 8 \
'NOTIFY_REQUEST_BLOCK_FLASHES|post_notify<NOTIFY_REQUEST_BLOCK_FLASHES>|heights\.push_back|heights\s*=' \
src tests --glob '*.h' --glob '*.hpp' --glob '*.cpp' --glob '*.inl' || true
printf '%s\n' '--- relevant limit definitions and uses ---'
rg -n -C 4 \
'CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT|CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT|NOTIFY_RESPONSE_BLOCK_FLASHES' \
src tests --glob '*.h' --glob '*.hpp' --glob '*.cpp' --glob '*.inl' || trueRepository: Beldex-Coin/beldex
Length of output: 49584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
handler = Path("src/cryptonote_protocol/cryptonote_protocol_handler.inl").read_text()
pool = Path("src/cryptonote_core/tx_pool.cpp").read_text()
m = re.search(
r'int\s+t_cryptonote_protocol_handler<[^>]+>::handle_request_block_flashes\b.*?'
r'\n\s*}\n\s*//-+\n\s*template<class t_core>\n\s*int\s+t_cryptonote_protocol_handler<[^>]+>::handle_response_block_flashes',
handler,
re.S,
)
assert m, "flash handler region not found"
request_handler = m.group(0)
assert "get_mined_flashes" in request_handler
assert "post_notify<NOTIFY_RESPONSE_BLOCK_FLASHES>(r, context)" in request_handler
assert not re.search(r'get_mined_flashes.*?CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT', request_handler, re.S)
m = re.search(
r'tx_memory_pool::get_mined_flashes\b.*?\n\s*}\n',
pool,
re.S,
)
assert m, "get_mined_flashes implementation not found"
producer = m.group(0)
assert "result.push_back(hashes[i])" in producer
assert "CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT" not in producer
assert not re.search(r'result\.size\(\).*?(?:5000|CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT)', producer, re.S)
print("handle_request_block_flashes forwards the complete get_mined_flashes result.")
print("get_mined_flashes appends every matching hash and has no 5000-item bound.")
print("The response handler rejects a response above CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT before its GET_TXS batching loop.")
PYRepository: Beldex-Coin/beldex
Length of output: 427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- flash-pool capacity and insertion limits ---'
rg -n -C 5 \
'max.*(flash|tx)|flash.*max|max_txpool|max_tx_pool|m_txpool_max_size|m_max.*pool|tx_pool_max|add.*flash|m_flashes|flash_hashes_and_mined_heights' \
src/cryptonote_core src/cryptonote_protocol --glob '*.h' --glob '*.hpp' --glob '*.cpp' --glob '*.inl' || true
printf '%s\n' '--- request handlers and request-size validation ---'
sed -n '288,306p' src/cryptonote_protocol/cryptonote_protocol_defs.h
sed -n '124,140p' src/cryptonote_protocol/cryptonote_protocol_defs.cpp
rg -n -C 15 \
'handle_request_block_flashes|arg\.heights\.size\(\)|NOTIFY_REQUEST_BLOCK_FLASHES' \
src/cryptonote_protocol --glob '*.h' --glob '*.hpp' --glob '*.cpp' --glob '*.inl' || true
printf '%s\n' '--- flash-related configuration ---'
rg -n -i -C 3 \
'flash.*(limit|count|size)|limit.*flash|flash.*pool|pool.*flash' \
src tests --glob '*.h' --glob '*.hpp' --glob '*.cpp' --glob '*.inl' --glob '*.conf' --glob '*.ini' || trueRepository: Beldex-Coin/beldex
Length of output: 50375
Limit or batch flash responses before posting them.
get_mined_flashes() returns every matching hash, and the flash pool has no 5000-item limit. If more than 5000 hashes match the requested heights, the receiver drops the connection before its batching loop. Limit or split NOTIFY_RESPONSE_BLOCK_FLASHES at the producer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cryptonote_protocol/cryptonote_protocol_handler.inl` around lines 2525 -
2531, Update the producer path that calls get_mined_flashes() and posts
NOTIFY_RESPONSE_BLOCK_FLASHES to cap each response at
CURRENCY_PROTOCOL_MAX_TXS_REQUEST_COUNT (5000) or split larger results into
multiple responses. Ensure every matching flash hash is delivered through
bounded batches so the receiver’s validation in cryptonote_protocol_handler does
not drop the connection.
sanada08
self-requested a review
August 12, 2026 05:12
sanada08
approved these changes
Aug 12, 2026
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
2 participants
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.
No description provided.