feat: implement new RPC getislocks and add hex field to RPC getbestchainlock #6455
Conversation
|
Guix Automation has began to build this PR tagged as v22.1.0-devpr6455.a4256c92. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.1.0-devpr6455.a4256c92. The image should be on dockerhub soon. |
knst
changed the title
|
Guix Automation has began to build this PR tagged as v22.1.0-devpr6455.756b7722. A new comment will be made when the image is pushed. |
WalkthroughThe pull request introduces modifications to the RPC interfaces related to blockchain and transaction handling in the Dash cryptocurrency implementation. The Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/rpc/rawtransaction.cpp (1)
369-420: LGTM! Implementation looks solid.The new RPC method is well-structured with proper input validation, error handling, and serialization.
Consider using CHECK_NONFATAL for llmq_ctx.isman
For better error handling consistency with the codebase:
-if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) { +if (const llmq::CInstantSendLockPtr islock = CHECK_NONFATAL(llmq_ctx.isman)->GetInstantSendLockByTxid(txid); islock != nullptr) {src/rpc/blockchain.cpp (1)
272-301: LGTM! Implementation looks solid.The new RPC method is well-structured with proper error handling and serialization.
Consider using RPC_INVALID_REQUEST for consistency
For better error code consistency with other RPC methods:
- throw JSONRPCError(RPC_MISC_ERROR, "Unable to find any ChainLock"); + throw JSONRPCError(RPC_INVALID_REQUEST, "Unable to find any ChainLock");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/rpc/blockchain.cpp(2 hunks)src/rpc/client.cpp(1 hunks)src/rpc/rawtransaction.cpp(3 hunks)test/functional/interface_zmq_dash.py(4 hunks)
🔇 Additional comments (4)
src/rpc/client.cpp (1)
118-118: LGTM!The new entry for
getrawislocksis correctly added to thevRPCConvertParamsarray, maintaining alphabetical order and following the established pattern.test/functional/interface_zmq_dash.py (1)
139-140: LGTM! Test coverage looks good.The added test assertions provide good coverage for both the new RPC methods:
- Tests error case for
getrawbestchainlockwhen no ChainLock exists- Tests both negative and positive cases for
getrawislocks(before and after InstantSend lock)Also applies to: 291-291, 311-311
src/rpc/rawtransaction.cpp (1)
1984-1984: LGTM! Command registration looks good.The new RPC command is properly registered in the command table.
src/rpc/blockchain.cpp (1)
2705-2705: LGTM! Command registration looks good.The new RPC command is properly registered in the command table.
as discussed, RPC will be changed:
|
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.1.0-devpr6455.756b7722. The image should be on dockerhub soon. |
knst
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/rpc/blockchain.cpp (1)
274-289: Consider adding error handling for serialization.The core logic is correct, but consider:
- Using a named constant for
PROTOCOL_VERSIONto improve maintainability- Adding error handling for potential serialization failures
- CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); + try { + CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); + ssTx << clsig; + return HexStr(ssTx); + } catch (const std::exception& e) { + throw JSONRPCError(RPC_INTERNAL_ERROR, + std::string("Failed to serialize ChainLock: ") + e.what()); + } - ssTx << clsig; - return HexStr(ssTx);src/rpc/rawtransaction.cpp (2)
431-432: Movellmq_ctxdeclaration outside the loop for better performance.The
LLMQContextis retrieved inside the loop but it could be moved outside since it's used for all transactions.Apply this diff to improve performance:
const NodeContext& node = EnsureAnyNodeContext(request.context); + const LLMQContext& llmq_ctx = EnsureLLMQContext(node); for (const auto idx : irange::range(txids.size())) { const uint256 txid(ParseHashV(txids[idx], "txid")); - const LLMQContext& llmq_ctx = EnsureLLMQContext(node);
435-456: Consider using structured bindings for better readability.The code can be made more readable by using C++17's structured bindings and early returns.
Apply this diff to improve readability:
- if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) { + if (auto islock = CHECK_NONFATAL(llmq_ctx.isman)->GetInstantSendLockByTxid(txid); islock != nullptr) { if (verbose == 1) { UniValue objIS(UniValue::VOBJ); objIS.pushKV("txid", islock->txid.ToString()); UniValue outputs(UniValue::VARR); for (const auto& out : islock->inputs) { UniValue outpoint(UniValue::VOBJ); outpoint.pushKV("txid", out.hash.ToString()); outpoint.pushKV("vout", static_cast<int64_t>(out.n)); outputs.push_back(outpoint); } objIS.pushKV("cycleHash", islock->cycleHash.ToString()); objIS.pushKV("sig", islock->sig.ToString()); result_arr.push_back(objIS); } else { CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << *islock; result_arr.push_back(HexStr(ssTx)); } } else { result_arr.push_back("None"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/rpc/blockchain.cpp(1 hunks)src/rpc/client.cpp(1 hunks)src/rpc/rawtransaction.cpp(3 hunks)test/functional/interface_zmq_dash.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/client.cpp
- test/functional/interface_zmq_dash.py
🔇 Additional comments (5)
src/rpc/blockchain.cpp (2)
235-251: LGTM! Documentation and parameter definition are clear and comprehensive.The updated documentation clearly describes:
- The new
verboseparameter and its default value- The different return formats based on the verbose flag
- The error condition when no ChainLock exists
259-266: LGTM! Parameter handling is robust and maintains backward compatibility.The code properly handles:
- Both boolean and numeric input types for the
verboseparameter- Default value of 1 for backward compatibility
src/rpc/rawtransaction.cpp (3)
2186-2186: LGTM!The new command is properly registered in the RPC table.
382-407: LGTM!The RPCResult documentation is comprehensive and well-structured, covering both output formats and error cases.
370-462: Implementation aligns with PR objectives.The
getislocksimplementation successfully:
- Provides InstantSend lock data in both verbose and raw formats
- Matches ZMQ output format when
verbose=0- Supports the IdentityCreateTransition process by providing InstantLock data
|
Guix Automation has began to build this PR tagged as v22.1.0-devpr6455.2346aa72. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/rpc/rawtransaction.cpp (1)
431-457: Consider performance optimization by moving context outside the loop.The
llmq_ctxis accessed within the loop for each transaction. Consider moving it outside to avoid repeated context retrieval.const LLMQContext& llmq_ctx = EnsureLLMQContext(node); +const auto& isman = llmq_ctx.isman; for (const auto idx : irange::range(txids.size())) { const uint256 txid(ParseHashV(txids[idx], "txid")); - if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) { + if (const llmq::CInstantSendLockPtr islock = isman->GetInstantSendLockByTxid(txid); islock != nullptr) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/rpc/client.cpp(1 hunks)src/rpc/rawtransaction.cpp(3 hunks)test/functional/interface_zmq_dash.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/functional/interface_zmq_dash.py
- src/rpc/client.cpp
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (2)
src/rpc/rawtransaction.cpp (2)
2186-2186: LGTM!The command registration follows the established pattern and is correctly placed in the command table.
435-435:⚠️ Potential issueAdd nullptr check for InstantSend manager.
Add a CHECK_NONFATAL for the InstantSend manager to ensure it's not null before dereferencing.
- if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) { + if (const llmq::CInstantSendLockPtr islock = CHECK_NONFATAL(llmq_ctx.isman)->GetInstantSendLockByTxid(txid); islock != nullptr) {Likely invalid or redundant comment.
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.1.0-devpr6455.2346aa72. The image should be on dockerhub soon. |
| for (const auto idx : irange::range(txids.size())) { | ||
| const uint256 txid(ParseHashV(txids[idx], "txid")); | ||
|
|
||
| if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) { |
There was a problem hiding this comment.
nit: could use const auto here
There was a problem hiding this comment.
I like to see a type here 🤷 but I have no strong opinion to remove it
src/rpc/rawtransaction.cpp
Outdated
| }, | ||
| }}, | ||
| {RPCResult::Type::STR_HEX, "cycleHash", "The Cycle Hash"}, | ||
| {RPCResult::Type::STR_HEX, "signature", "The InstantSend's BLS signature"}, |
There was a problem hiding this comment.
Don't think we should mention the algorithm used to generate the signature in the help text.
| {RPCResult::Type::STR_HEX, "signature", "The InstantSend's BLS signature"}, | |
| {RPCResult::Type::STR_HEX, "signature", "The InstantSend lock's signature"}, |
There was a problem hiding this comment.
unified with getbestchainlock
{RPCResult::Type::STR_HEX, "signature", "The ChainLock's BLS signature"},
There was a problem hiding this comment.
That should be changed too IMO but change could be out-of-scope for this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/rpc/rawtransaction.cpp (3)
380-380: Use bool type for verbose parameter to maintain consistency.The verbose parameter is defined as RPCArg::Type::NUM, but other RPCs in the codebase use RPCArg::Type::BOOL for verbose flags. Consider changing to maintain consistency.
- {"verbose", RPCArg::Type::NUM, RPCArg::Default{1}, "0 for hex-encoded data, 1 for a json object"}, + {"verbose", RPCArg::Type::BOOL, RPCArg::Default{true}, "false for hex-encoded data, true for a json object"},
435-435: Use const auto for better readability and const correctness.The
islockvariable should be declared withconst autoto improve readability and enforce const correctness.- if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) { + if (const auto islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) {
402-402: Update signature field name to match help text.The help text refers to "signature" but the code uses "sig". Consider updating for consistency.
- {RPCResult::Type::STR_HEX, "signature", "The InstantSend's BLS signature"}, + {RPCResult::Type::STR_HEX, "sig", "The InstantSend's BLS signature"},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/rpc/client.cpp(1 hunks)src/rpc/rawtransaction.cpp(3 hunks)test/functional/interface_zmq_dash.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/client.cpp
- test/functional/interface_zmq_dash.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (2)
src/rpc/rawtransaction.cpp (2)
2187-2188: LGTM! Command registration follows the established pattern.The registration of the new
getislockscommand is properly implemented and follows the existing pattern in the codebase.
445-445:⚠️ Potential issueAdd missing inputs array to object.
The inputs array is not being pushed to the object in the JSON output.
} + objIS.pushKV("inputs", inputs);Likely invalid or redundant comment.
| const uint256 txid(ParseHashV(txids[idx], "txid")); | ||
|
|
||
| if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid transaction IDs.
The function should validate transaction IDs before attempting to retrieve InstantSend locks.
const uint256 txid(ParseHashV(txids[idx], "txid"));
+ if (txid.IsNull()) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid txid at index %d", idx));
+ }
if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) {📝 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.
| const uint256 txid(ParseHashV(txids[idx], "txid")); | |
| if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) { | |
| const uint256 txid(ParseHashV(txids[idx], "txid")); | |
| if (txid.IsNull()) { | |
| throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid txid at index %d", idx)); | |
| } | |
| if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) { |
It adds ability to receive raw chainlock in hex format (compatible with ZMQ subscription)
knst
changed the title
|
Guix Automation has began to build this PR tagged as v22.1.0-devpr6455.fd1e774b. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/rpc/rawtransaction.cpp (2)
372-373: Fix inconsistency in help text.The help text uses "getislocks" but the actual function name in the code is "getislocks". This should be consistent.
- "\nReturns the raw InstantSend lock data for each txids. Returns Null if there is no known IS yet.", + "\nReturns the raw InstantSend lock data for each transaction ID. Returns 'None' if there is no known InstantSend lock yet.",
418-443: Use range-based for loop for better readability.The current loop using
irange::rangecan be simplified using a range-based for loop.- for (const auto idx : irange::range(txids.size())) { - const uint256 txid(ParseHashV(txids[idx], "txid")); + for (const auto& txid_value : txids.getValues()) { + const uint256 txid(ParseHashV(txid_value, "txid"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/rpc/blockchain.cpp(1 hunks)src/rpc/client.cpp(1 hunks)src/rpc/rawtransaction.cpp(3 hunks)test/functional/interface_zmq_dash.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/rpc/client.cpp
- src/rpc/blockchain.cpp
- test/functional/interface_zmq_dash.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Validate PR title
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (2)
src/rpc/rawtransaction.cpp (2)
2172-2172: LGTM!The command registration follows the existing pattern and is correctly placed in the registration array.
419-421:⚠️ Potential issueAdd error handling for invalid transaction IDs.
The function should validate transaction IDs before attempting to retrieve InstantSend locks.
const uint256 txid(ParseHashV(txids[idx], "txid")); + if (txid.IsNull()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid txid at index %d", idx)); + } if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) {Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/rpc/rawtransaction.cpp (1)
433-438: Consider using a more efficient serialization approach.The current implementation creates a new
CDataStreamfor each InstantSend lock. Consider moving the stream creation outside the loop if multiple transactions are expected.Apply this diff to improve serialization efficiency:
+ CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); { - CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << *islock; objIS.pushKV("hex", HexStr(ssTx)); + ssTx.clear(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/rpc/client.cpp(1 hunks)src/rpc/rawtransaction.cpp(3 hunks)test/functional/interface_zmq_dash.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/client.cpp
- test/functional/interface_zmq_dash.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (3)
src/rpc/rawtransaction.cpp (3)
413-415: LGTM! Input validation looks good.The size limit check for the input array is properly implemented.
2172-2173: LGTM! Command registration is correct.The new
getislockscommand is properly registered in the RPC table.
419-421: 🛠️ Refactor suggestionAdd error handling for invalid transaction IDs.
While the code parses transaction IDs, it should validate them before attempting to retrieve InstantSend locks.
Apply this diff to add validation:
const uint256 txid(ParseHashV(txids[idx], "txid")); + if (txid.IsNull()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid txid at index %d", idx)); + } if (const llmq::CInstantSendLockPtr islock = llmq_ctx.isman->GetInstantSendLockByTxid(txid); islock != nullptr) {Likely invalid or redundant comment.
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.1.0-devpr6455.fd1e774b. The image should be on dockerhub soon. |
The field hex contains hex-encoded binary representation which is compatible with zmq-subscription
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/rpc/rawtransaction.cpp (2)
373-373: Improve command description.The description should provide more context about InstantSend locks and their purpose.
- "\nReturns the raw InstantSend lock data for each txids. Returns Null if there is no known IS yet.", + "\nReturns the raw InstantSend lock data for each transaction ID. InstantSend locks provide instant transaction security\n" + "by locking transaction inputs. Returns 'None' if there is no known InstantSend lock for a transaction.",
435-438: Consider memory usage for large responses.The serialization of large InstantSend locks could consume significant memory. Consider adding a warning in the help text about memory usage with large numbers of transactions.
test/functional/interface_zmq_dash.py (2)
139-140: Add more test cases for getbestchainlock.The test only covers the error case. Consider adding tests for:
- Successful chainlock retrieval
- Verification of the hex field format
- Edge cases with multiple competing chainlocks
Line range hint
291-312: Add error case tests for getislocks.While the happy path is well tested, consider adding tests for:
- Invalid transaction IDs
- Maximum limit of 100 transactions
- Malformed transaction IDs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/rpc/client.cpp(1 hunks)src/rpc/rawtransaction.cpp(3 hunks)test/functional/interface_zmq_dash.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rpc/client.cpp
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (1)
src/rpc/rawtransaction.cpp (1)
2172-2172: LGTM!The RPC command registration is correct and follows the established pattern.
| const LLMQContext& llmq_ctx = EnsureLLMQContext(node); | ||
| for (const auto idx : irange::range(txids.size())) { |
There was a problem hiding this comment.
Add null check for llmq_ctx.isman.
The code directly accesses llmq_ctx.isman without checking if it's null.
const LLMQContext& llmq_ctx = EnsureLLMQContext(node);
+if (!llmq_ctx.isman) {
+ throw JSONRPCError(RPC_INTERNAL_ERROR, "InstantSend manager not available");
+}📝 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.
| const LLMQContext& llmq_ctx = EnsureLLMQContext(node); | |
| for (const auto idx : irange::range(txids.size())) { | |
| const LLMQContext& llmq_ctx = EnsureLLMQContext(node); | |
| if (!llmq_ctx.isman) { | |
| throw JSONRPCError(RPC_INTERNAL_ERROR, "InstantSend manager not available"); | |
| } | |
| for (const auto idx : irange::range(txids.size())) { |
Issue being fixed or feature implemented
#6391
What was done?
To retrieve information about ChainLocks has been implemented new field
hexfor RPC getbestchainlock which return information in zmq-compatible hex-encoded binary format.To retrieve information about InstantSend Lock has been implemented a new RPC
getislocksthat return information for list of txids in human-friendly JSON format and binary hex-encoded zmq-compatible format.How Has This Been Tested?
See new checks in functional test
interface_zmq_dash.pyBreaking Changes
N/A
Checklist: