rpc/wallet: Add details and duplicate section for simulaterawtransaction #25621
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
anibilthare
force-pushed
the
202108-analyzerawtransaction
branch
from
a1b6c49 to
ec67358
Compare
anibilthare
force-pushed
the
202108-analyzerawtransaction
branch
from
decf4e4 to
bf9deb4
Compare
achow101
left a comment
achow101
left a comment
There was a problem hiding this comment.
Note: @anibilthare is my Summer of Bitcion mentee.
src/wallet/rpc/wallet.cpp
Outdated
There was a problem hiding this comment.
In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"
nit: Unrelated whitespace change
src/wallet/rpc/wallet.cpp
Outdated
There was a problem hiding this comment.
In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"
nit: Unrelated whitespace change
src/wallet/rpc/wallet.cpp
Outdated
There was a problem hiding this comment.
In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"
This comment is a little confusing to understand.
| // if user tries to double spend in the provided transactions themselves then without this condition we will get an inappropirate error | |
| // Only apply changes the first time a transaction is seen. Otherwise duplicate transactions will give incorrect errors. |
There was a problem hiding this comment.
In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"
Unrelated change
There was a problem hiding this comment.
In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"
nit: Extra blank lines can be removed.
There was a problem hiding this comment.
In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"
nit: Extra blank lines can be removed.
src/wallet/rpc/wallet.cpp
Outdated
There was a problem hiding this comment.
In bf9deb407a8d73ac3b645b34c9bd6d28729a809c "rpc/wallet: Add details and duplicate section for simulaterawtransaction"
txid is sufficient
| {RPCResult::Type::STR_HEX, "txid_in_wallet", "the txid of conflicting transactions in wallet"}, | |
| {RPCResult::Type::STR_HEX, "txid", "the txid of conflicting transactions in wallet"}, |
anibilthare
force-pushed
the
202108-analyzerawtransaction
branch
from
bf9deb4 to
c5087b3
Compare
|
Can be rebased now that #22751 is merged. |
josibake
left a comment
josibake
left a comment
There was a problem hiding this comment.
Concept ACK
Nice work with informing the user of conflicts! Seems to definitely improve the usability of the RPC. I tried rebasing on master and the rebase wasn't trivial, so I'll wait until you're able to rebase to do a more thorough code review. I left a few comments on the design of the details object and a few nits on the code. In general, I'd avoid making stylistic changes (adding/removing whitespace) and avoid adding/editing comments on the existing code.
Lastly, it might help to break this up into a few smaller commits to make it easier to review. I'd suggest putting the refactors (renaming variables, refactoring GetConflicts) into their own refactor commit and a separate commit for the new functionality.
src/wallet/rpc/wallet.cpp
Outdated
There was a problem hiding this comment.
Why are we returning indices in simulated_tx_conflicts instead of txids? As an end user, I think I'd prefer to have a consistent schema for the details object where both *_conflicts fields have txids.
There was a problem hiding this comment.
Returning txids increases the "manual" work on user's side, if that makes sense? Consider a case when user provides 50 transactions as input. As an end user I think I'd prefer the software telling me "This transaction of yours conflicts with the transaction 11, 17 and 21 that you provided" rather than "This transaction of yours conflict with the transactions with txids "
src/wallet/rpc/wallet.cpp
Outdated
There was a problem hiding this comment.
I don't understand why this is needed. Wouldn't it be better to have the RPC fail fast and tell the user they are providing duplicates?
There was a problem hiding this comment.
I did consider what you suggested but let's say we do that, then the user will remove those duplicate transactions from the list and re run the RPC? To me this seemed redundant and that's why I took this approach.
src/wallet/rpc/wallet.cpp
Outdated
There was a problem hiding this comment.
nit: adding comments which restate what existing code is doing aren't super valuable imo and can be distracting when trying to review the code changes
src/wallet/rpc/wallet.cpp
Outdated
The currently provided solution implements an RPC that iterates over the provided list of raw transactions and provides users with the balance change that the transactions will cause in the wallet but the current implementation does not deal with the conflicts that might arise while executing the transactions. This implementation is a follow up for the same, this implementation provides following enhancement to the RPC - For every transaction, say tx1, in the input list, a list of unconfirmed transactions in the wallet which conflict with tx1 - For every transaction, say tx2, in the input list, a list of transactions in the same list which conflict with tx2 - In case the user provides a transaction more than once, after first occurrence, rest occurrences are ignored and the index(0-based) for such transactions is reported to the user See also: PR#22751
anibilthare
force-pushed
the
202108-analyzerawtransaction
branch
from
c5087b3 to
d969370
Compare
|
@anibilthare are you still working on this? Can you comment where you've addressed the review feedback. Note that at least one test is also failing. |
DrahtBot
changed the title
6 participants
The currently provided solution implements a RPC that iterates over the provided list of raw transactions and provides users with the balance change that the transactions will cause in the wallet but the current implementation does not deal with the conflicts that might arise while executing the transactions. This implementation is a follow up for the same, this implementation provides following enhancements to the RPC
- For every transaction, say tx1, in the input list, a list of unconfirmed transactions in the wallet which conflict with tx1
- For every transaction, say tx2, in the input list, a list of transactions in the same list which conflict with tx2
- In case the user provides a transaction more than once, after the first occurrence, the rest of the occurrences are ignored and the index(0-based) for such transactions is reported to the user
See also: PR#22751