The current logic is a bit confusing. First creating the UniValue return dict, then parsing it again to get the type as std::string.
Clean this up by using a strong type TxoutType. Also, remove whitespace.
refactor: Call type-solver earlier in decodescript #23642
pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2112-rpcRefDec changing 1 files +31 −32-
MarcoFalke commented at 10:12 AM on December 1, 2021: member
-
fab0d998f4
style: Remove whitespace
Can be reviewed via --ignore-all-space
-
3333070208
refactor: Call type-solver earlier in decodescript
Also, remove std::string type.
- MarcoFalke added the label Refactoring on Dec 1, 2021
- MarcoFalke added the label RPC/REST/ZMQ on Dec 1, 2021
- shaavan approved
-
shaavan commented at 11:15 AM on December 1, 2021: contributor
ACK 33330702081f67cb05fd86e00b252f6355249513
The refactoring looks good, and I find it an improvement over the current master’s code. This PR removes the extra steps of:
- Searching for the ‘type’ value of the script and changing it to str, and,
- Checking and comparing the type values in str format.
Instead, this PR directly creates a TxoutType object corresponding to the script and compares the result with the enum values of the TxoutType class, which was subsequently done even in the master branch’s case.
The whitespace removal commit also looks good. I checked for any formatting issues, and I found none.
-
DrahtBot commented at 1:11 PM on December 1, 2021: member
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments [WIP; TOO MANY CONFLICTS] by MarcoFalke)
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.
-
theStack commented at 7:08 PM on December 1, 2021: member
Concept ACK on using the strong type
TxoutTypeinstead of strings. However, now the type-solving happens rather twice (once inScriptPubKeyToUniv, a second time immediately after its call) rather than "earlier", isn't it? Not sure though if this is really a problem for the performance, probably the RPC call overhead is already an order of magnitude higher than the solving itself, so that it doesn't really matter if it's done twice. -
MarcoFalke commented at 7:51 AM on December 2, 2021: member
Yes, I'd say it shouldn't matter performance wise. After all, it already happens twice on current master.
- theStack approved
-
theStack commented at 12:48 PM on December 2, 2021: member
Yes, I'd say it shouldn't matter performance wise. After all, it already happens twice on current master.
Oh, nevermind, for some reason I didn't see the second solver call in master 🤦♂️
Code-review ACK 33330702081f67cb05fd86e00b252f6355249513
- MarcoFalke referenced this in commit 0bd7ca9a03 on Dec 2, 2021
- MarcoFalke closed this on Dec 2, 2021
- MarcoFalke deleted the branch on Dec 2, 2021
- sidhujag referenced this in commit 2f3e4b502b on Dec 2, 2021
- RandyMcMillan referenced this in commit ea8e28bb33 on Dec 23, 2021
- DrahtBot locked this on Dec 2, 2022
Contributors
Labels