-
-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New rpc calls for mobile #1449
New rpc calls for mobile #1449
Conversation
WalkthroughThe recent changes encompass various enhancements to the RPC methods and the handling of transaction hashes within the blockchain. Key updates include new RPC commands for retrieving mempool transaction IDs and tags, modifications to the Changes
Sequence Diagrams
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/chain.h (3 hunks)
- src/rpc/client.cpp (1 hunks)
- src/rpc/misc.cpp (3 hunks)
- src/rpc/server.cpp (1 hunks)
- src/rpc/server.h (1 hunks)
- src/spark/state.cpp (5 hunks)
- src/spark/state.h (4 hunks)
- src/txdb.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/rpc/client.cpp
Additional comments not posted (17)
src/rpc/server.h (3)
219-219
: The declaration ofgetusedcoinstagstxhashes
is correctly added.
221-221
: The declaration ofgetmempooltxids
is correctly added.
222-222
: The declaration ofgetmempooltxs
is correctly added.src/spark/state.h (3)
29-29
: The addition ofltagTxhash
member inCSparkState
is correctly implemented to store linking tag hashes mapped to transaction hashes.
172-172
: The addition ofAddLTagTxHash
method inCSparkState
is correctly implemented to handle the insertion of linking tag hashes.
218-218
: The addition ofGetSpendTxIds
method inCSparkState
is correctly implemented to retrieve linking tag hashes.src/rpc/server.cpp (3)
345-345
: The addition ofgetusedcoinstagstxhashes
command in the mobile category is correctly implemented.
347-347
: The addition ofgetmempooltxids
command in the mobile category is correctly implemented.
348-348
: The addition ofgetmempooltxs
command in the mobile category is correctly implemented.src/chain.h (2)
264-265
: The addition ofltagTxhash
toCBlockIndex
is well-implemented. Ensure that the hash map is used securely and efficiently.
308-308
: ClearingltagTxhash
in theSetNull
method is a good practice to ensure that the object starts with a clean state.src/txdb.cpp (1)
428-428
: The assignment ofltagTxhash
fromdiskindex
topindexNew
ensures that the transaction hash map is correctly loaded into the block index. This is crucial for the new RPC functionalities introduced in the PR.Verification successful
The
ltagTxhash
is correctly utilized insrc/rpc/misc.cpp
for mapping transaction IDs, which aligns with the intended functionality. The assignment insrc/txdb.cpp
ensures that this mapping is correctly loaded into the block index.
src/rpc/misc.cpp: std::unordered_map<uint256, uint256> ltagTxhash;
src/rpc/misc.cpp: ltagTxhash = sparkState->GetSpendTxIds();
src/rpc/misc.cpp: if (ltagTxhash.count(ltagHash) > 0)
src/rpc/misc.cpp: txid = ltagTxhash[ltagHash];
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ltagTxhash` is correctly utilized in the new RPC methods. # Test: Search for the usage of `ltagTxhash` in the new RPC methods. Expect: Proper handling and no errors. rg --type cpp $'ltagTxhash' src/rpc/Length of output: 291
src/spark/state.cpp (5)
268-277
: The handling of-mobile
flag for conditional execution is consistent across the codebase.
1090-1094
: The conditional logic for handling the-mobile
flag during block addition is implemented correctly. This ensures that mobile-specific data is handled appropriately based on runtime configuration.
1341-1343
: The methodGetSpendTxIds
provides a consistent interface to accessltagTxhash
. This is a good practice as it encapsulates the data structure, allowing for easier maintenance and potential future changes.
1050-1052
: The methodAddLTagTxHash
is added to handle the insertion of tag-hash pairs intoltagTxhash
. Ensure that this method is used consistently whereverltagTxhash
is modified.Verification successful
The search results indicate only one direct modification to
ltagTxhash
, which is within theAddLTagTxHash
method itself. This confirms that the method is used consistently for modifyingltagTxhash
.
src/spark/state.cpp
: The direct modification found is within theAddLTagTxHash
method.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of `AddLTagTxHash` method across the codebase. # Test: Search for direct modifications to `ltagTxhash` that bypass `AddLTagTxHash`. rg --type cpp $'ltagTxhash\[.*\] ='Length of output: 91
702-704
: The conditional insertion ofltagTxhash
based on the-mobile
flag is implemented correctly. However, ensure that the functionprimitives::GetLTagHash
is robust and handles edge cases effectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/rpc/misc.cpp (3 hunks)
Additional comments not posted (1)
src/rpc/misc.cpp (1)
1510-1588
: The implementation ofgetmempooltxs
appears to be correct, but ensure that the method is thoroughly tested, especially the parsing and handling of transaction data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add the description of what's the problem and how it's fixed. I see the coderabbit's comments, but this is not enough to do the review. Especially please explain the reasons behind adding new members of CBlockIndex
.
@psolstice the description added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/rpc/misc.cpp (3 hunks)
- src/rpc/server.cpp (1 hunks)
- src/rpc/server.h (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/rpc/server.cpp
Additional comments not posted (4)
src/rpc/server.h (1)
221-221
: Ensure consistency in naming and functionality forgetmempoolsparktxids
.This function is supposed to return all spark transaction IDs currently in the mempool. Verify its implementation in
src/rpc/misc.cpp
to ensure it aligns with the naming conventions and functionality described.Verification successful
The
getmempoolsparktxids
function insrc/rpc/misc.cpp
is correctly implemented to return all spark transaction IDs currently in the mempool. It checks for help requests or incorrect parameters, retrieves all transactions from the mempool, filters for spark transactions, and returns their IDs.
- The function name
getmempoolsparktxids
accurately reflects its functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of `getmempoolsparktxids` in `src/rpc/misc.cpp` fd "misc.cpp" src/rpc/ --exec cat {} | grep -A 3 -B 3 "getmempoolsparktxids"Length of output: 904
Script:
#!/bin/bash # Retrieve the full implementation of the `getmempoolsparktxids` function in `src/rpc/misc.cpp` ast-grep --lang cpp --pattern 'UniValue getmempoolsparktxids(const JSONRPCRequest& request) { $$$ }' src/rpc/misc.cppLength of output: 119
Script:
#!/bin/bash # Reattempt to retrieve the full implementation of the `getmempoolsparktxids` function in `src/rpc/misc.cpp` rg -A 20 -B 5 'UniValue getmempoolsparktxids' src/rpc/misc.cppLength of output: 769
src/rpc/misc.cpp (3)
1407-1462
: Exception handling forstd::stol
is correctly implemented, ensuring robust input validation.
1491-1508
: Proper implementation ofgetmempoolsparktxids
to retrieve Spark transaction IDs from the mempool.
1510-1588
: Consider clarifying the RPC help description forgetmempoolsparktxs
to better explain the expected input and output data formats.
[ISSU- "\nReturns spark data for each transaction.\n" + "\nReturns detailed Spark transaction data for each specified transaction ID.\n"
@@ -216,7 +216,10 @@ extern UniValue getlatestcoinid(const JSONRPCRequest& params); | |||
extern UniValue getsparkanonymityset(const JSONRPCRequest& params); | |||
extern UniValue getsparkmintmetadata(const JSONRPCRequest& params); | |||
extern UniValue getusedcoinstags(const JSONRPCRequest& params); | |||
extern UniValue getusedcoinstagstxhashes(const JSONRPCRequest& params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add documentation for getusedcoinstagstxhashes
.
The function getusedcoinstagstxhashes
is introduced to extend the functionality of the existing getusedcoinstags
by also providing the transaction hash in which the coin was spent. It's crucial to have proper documentation for new RPC methods to ensure they are used correctly and understood by other developers or API consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added new rpc calls,
getusedcoinstagstxhashes
, this rpc returns all spent coin serials attached to the hash of spending transaction, previously we hadgetusedcoinstags
, the new call allows to get also in which transaction is spent the coin, to do this we add field inside block index, and in spark state, when tx is being checked during block acceptance, we add the pair of ltag and tx hash into block index, and each time we construct the state, we get the data from the index. this fields are constructed in case-mobile
is passed,getmempooltxids
gives the list of spark tx hashes inside mempool, this allow to get spark state not only for blockchian but also from mempool.getmempooltxs
returns spark metadata for each tx hash, existing in mempool, in case there is no data for particular tx, that means tx is removed from mempool, confirmed or removed for another reason.