feat: implement new RPCs getislocks and add verbose=0 to 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 significant modifications to the RPC interfaces related to blockchain and transaction handling in the Dash cryptocurrency implementation. It updates 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. |
It adds ability to receive raw chainlock in hex format (compatible with ZMQ subscription)
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. |
| "If verbose is 1, returns information about the best ChainLock.\n" | ||
| "Throws an error if there is no known ChainLock yet.", | ||
| { | ||
| {"verbose", RPCArg::Type::NUM, RPCArg::Default{1}, "0 for hex-encoded data, 1 for a json object"}, |
There was a problem hiding this comment.
nit: could be bool instead (unless there are plans to allow greater verbosity in the future)
There was a problem hiding this comment.
see RPC getblock.
It's done for convinience, so, you can call getbestchainlock true or gestbestchainlock 1 - both ways will work
There was a problem hiding this comment.
verbose should be bool. If you want int it should be verbosity.
You can achieve the same effect (true/1) for bool by doing it the way it's done in getrawtransaction i.e.
bool fVerbose = false;
if (!request.params[0].isNull()) {
fVerbose = request.params[0].isNum() ? (request.params[0].get_int() != 0) : request.params[0].get_bool();
}
| 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
| }, | ||
| }}, | ||
| {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.
| {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "A transaction hash"}, | ||
| }, | ||
| }, | ||
| {"verbose", RPCArg::Type::NUM, RPCArg::Default{1}, "0 for hex-encoded data, 1 for a json object"}, |
There was a problem hiding this comment.
Same suggestion like for getbestchainlock
It return InstantSend Lock as JSON or as hex-string depends on verbose=0|1 Hex format is compatible with zmq-subscription
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) { |
| "If verbose is 1, returns information about the best ChainLock.\n" | ||
| "Throws an error if there is no known ChainLock yet.", | ||
| { | ||
| {"verbose", RPCArg::Type::NUM, RPCArg::Default{1}, "0 for hex-encoded data, 1 for a json object"}, |
There was a problem hiding this comment.
verbose should be bool. If you want int it should be verbosity.
You can achieve the same effect (true/1) for bool by doing it the way it's done in getrawtransaction i.e.
bool fVerbose = false;
if (!request.params[0].isNull()) {
fVerbose = request.params[0].isNum() ? (request.params[0].get_int() != 0) : request.params[0].get_bool();
}
| }, | ||
| { | ||
| RPCResult{"for verbose = 0", | ||
| RPCResult::Type::STR, "data", "The serialized, hex-encoded data for best ChainLock"}, |
There was a problem hiding this comment.
| RPCResult::Type::STR, "data", "The serialized, hex-encoded data for best ChainLock"}, | |
| RPCResult::Type::STR_HEX, "data", "The serialized, hex-encoded data for best ChainLock"}, |
Do we need known_block here too? In this case we could probably just add a new hex field to the old RPC results instead of adding a new param.
| {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "A transaction hash"}, | ||
| }, | ||
| }, | ||
| {"verbose", RPCArg::Type::NUM, RPCArg::Default{1}, "0 for hex-encoded data, 1 for a json object"}, |
There was a problem hiding this comment.
Same suggestion like for getbestchainlock
| RPCResult::Type::ARR, "", "Response is an array with the same size as the input txids", | ||
| { | ||
| RPCResult{"for verbose = 0 if InstantSend Lock is known for specified txid", | ||
| RPCResult::Type::STR, "data", "The serialized, hex-encoded data for 'txid'" |
There was a problem hiding this comment.
| RPCResult::Type::STR, "data", "The serialized, hex-encoded data for 'txid'" | |
| RPCResult::Type::STR_HEX, "data", "The serialized, hex-encoded data for 'txid'" |
Issue being fixed or feature implemented
#6391
What was done?
To retrieve information about ChainLocks has been implemented new param
verbose=0|1[default]for RPC getbestchainlock which return information in zmq-compatible hex format.To retrieve information about InstantSend Lock has been implemented a new RPC
getrawislocksthat return information for list of txids in human-friendly JSON format and zmq-compatible hex format.How Has This Been Tested?
See new checks in functional test
interface_zmq_dash.pyBreaking Changes
N/A
Checklist: