refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag #22924

mjdietzx · 2021-09-08T17:12:34Z

This is a follow-up on top of #22650. Based on review there, we thought it would be an improvement to merge ScriptPubKeyToUniv and ScriptToUniv into a single function, but this didn't seem to belong in that PR. There are a few other minor cleanups here as well focused on style and making some fuzz tests run faster

mjdietzx · 2021-09-14T20:28:01Z

Marking this as a draft for now. I will be moving some of the refactoring/cleanup that was originally included in #22650 to this PR. So once that is done and I've rebased this PR, I will reopen it

mjdietzx · 2021-09-29T03:15:40Z

Now that #22650 is merged, I rebased and finished this. This PR follows up with some cleanup, and addresses all the comments in the first PR that were left for now.

Just so we have them at-hand, I took all the original comments from #22650's review, and am putting them here.

@jonatack: maybe I'm missing something but perhaps merge ScriptPubKeyToUniv and ScriptToUniv into a single function

Done b9ad1d9

@jonatack: Named args could be nice wherever this function is invoked

and

@MarcoFalke: If you decide to do it here, at least use the proper clang-tidy format, so that it can be checked by the clang compiler

Done e9f3f1b

@MarcoFalke: as a follow-up it could make sense to de-duplicate the common help text

Done fe71ac8

@MarcoFalke: I think you can drop one of these statements to make the fuzz test run ~twice as fast. An alternative (to test both code paths) would be to deserialize a bool from the fuzzing input and use uint256{bool_val}

and

@meshcollider: This should really be replaced with include_address=true/false checks on ScriptPubKeyToUniv(script, o4, fIncludeHex, include_address)

Done e749168

src/test/fuzz/transaction.cpp

src/wallet/rpcwallet.cpp

src/test/fuzz/transaction.cpp

DrahtBot · 2021-09-29T20:42:05Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23599 (Tidy up RPCTxSerializationFlags by MarcoFalke)
#23546 (scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) by MarcoFalke)
#23486 (rpc: Only allow specific types to be P2(W)SH wrapped in decodescript by MarcoFalke)
#23320 (rpc: Add RPC help for getblock verbosity level 3 by kiminuo)
#23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)

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.

mjdietzx · 2021-10-25T15:24:15Z

Rebased, fixed some merge conflicts from #22918

src/core_write.cpp

laanwj · 2021-11-10T15:27:16Z

src/core_io.h

-void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include_hex, bool include_address = true);
-void ScriptToUniv(const CScript& script, UniValue& out);
-void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_DETAILS);
+void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex = true, bool include_address = false);


I wonder why we don't return the UniValue (or optional<UniValue> in case it can fail) instead of having it as second parameter. It would seem better API design to me.

I think this is a great suggestion. And at first glance it seems to be straightforward enough to include in this PR. I'll follow up shortly

I am leaning towards not including this change in this PR. I do think it is a great opportunity to cleanup and improve the code quality, so I would like to follow up with another PR doing this. This pattern is also used all over the place, not just ScriptToUniv, but TxToUniv, entryToJSON, and probably more. Anyways I want to spend some more time on this and don't want to end up doing too much in this PR

Can you take a quick look at the last two commits in this draft PR #23507?

a) is this along the lines of what you are thinking?
b) do you think it is worthwhile for me to do this across the codebase in #23507, eg entryToJSON, TxToJSON, etc... can be improved in the same way

Feel free to answer those in the linked PR and not here

mjdietzx · 2021-11-13T20:11:43Z

Addressed indentation nit and rebased to master

maflcko · 2021-12-01T10:09:24Z

src/rpc/blockchain.cpp

@@ -1274,7 +1274,7 @@ static RPCHelpMan gettxout()
                    {RPCResult::Type::STR, "asm", ""},
                    {RPCResult::Type::STR_HEX, "hex", ""},
                    {RPCResult::Type::STR, "type", "The type, eg pubkeyhash"},
-                    {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},
+                    {RPCResult::Type::STR, "address", /* optional */ true, DESTINATION_ADDRESS},


it might be better to de-duplicate the whole asm/hex/type/address blob. Though, maybe create a separate pull for doc cleanups vs code refactorings?

See for example e7507f3 or f8911de on how to do this.

I think this is a good suggestion. I will do this as a followup as you suggest. So I will get rid of 63a657c when I rebase and have a doc cleanup followup with these suggestions

DrahtBot · 2021-12-01T11:03:50Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2022-03-21T13:06:18Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko · 2022-03-22T11:20:49Z

I guess it is hard to tell how much of this is still relevant without doing a rebase?

fanquake · 2022-03-25T14:53:04Z

I've rebased some of the commits here in #24673. Haven't done 63a657c as we might be able to follow up differently there, and do some de-duplication, see #22924 (comment).

mjdietzx · 2022-03-25T15:06:39Z

Thanks @fanquake - it seems like the best path forward is closing this PR in favor of #24673? If so I'll close this and test/review the new PR

fanquake · 2022-03-25T15:08:49Z

Thanks @fanquake - it seems like the best path forward is closing this PR in favor of #24673? If so I'll close this and test/review the new PR

Sure, lets do that for now, and then potentially follow up with #23507 etc.

9563a64 refactor: add stdd:: includes to core_write (fanquake) 8b9efeb refactor: use named args when ScriptToUniv or TxToUniv are invoked (Michael Dietz) 22f25a6 refactor: prefer snake case, TxToUniv arg hashBlock renamed block_hash (Michael Dietz) 828a094 refactor: merge ScriptPubKeyToUniv & ScriptToUniv into one function (Michael Dietz) Pull request description: I've cherry-picked some of the commits out of #22924, and made minor changes (like fixing named args). ACKs for top commit: MarcoFalke: re-ACK 9563a64 🕓 Tree-SHA512: 4f0e5b45c14cbf68b9e389bbe1211c125d95cbd3da5205b1cff6a4c44f15b15039ba2a5b25cd7e2580d9169404f1b7ff620d8a7e01f6112e3cb153ecfaef8916

DrahtBot added Refactoring RPC/REST/ZMQ Utils/log/libs Wallet Needs rebase labels Sep 8, 2021

mjdietzx changed the title ~~refactor: merge ScriptPubKeyToUniv and ScriptToUniv into a single function~~ refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag Sep 14, 2021

mjdietzx marked this pull request as draft September 14, 2021 20:26

mjdietzx mentioned this pull request Sep 28, 2021

Remove -deprecatedrpc=addresses flag and corresponding code/logic #22650

Merged

mjdietzx force-pushed the refactor_ScriptToUniv branch 2 times, most recently from 1e8c0c5 to e749168 Compare September 29, 2021 01:48

fanquake removed the Needs rebase label Sep 29, 2021

mjdietzx marked this pull request as ready for review September 29, 2021 03:15

maflcko approved these changes Sep 29, 2021

View reviewed changes

src/test/fuzz/transaction.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

maflcko added Needs rebase and removed Wallet RPC/REST/ZMQ Utils/log/libs labels Sep 29, 2021

DrahtBot removed the Needs rebase label Sep 29, 2021

mjdietzx force-pushed the refactor_ScriptToUniv branch from e749168 to 08543bf Compare September 29, 2021 15:13

maflcko reviewed Sep 29, 2021

View reviewed changes

src/test/fuzz/transaction.cpp Outdated Show resolved Hide resolved

mjdietzx force-pushed the refactor_ScriptToUniv branch from 08543bf to 1402f0d Compare September 29, 2021 15:21

This was referenced Sep 30, 2021

Implement BIP 370 PSBTv2 #21283

Open

rpc: Add level 3 verbosity to getblock RPC call (#21245 modified) #22918

Merged

DrahtBot added the Needs rebase label Oct 19, 2021

mjdietzx force-pushed the refactor_ScriptToUniv branch from 1402f0d to 570cbb7 Compare October 25, 2021 15:22

DrahtBot removed the Needs rebase label Oct 25, 2021

laanwj reviewed Nov 10, 2021

View reviewed changes

src/core_write.cpp Outdated Show resolved Hide resolved

laanwj reviewed Nov 10, 2021

View reviewed changes

DrahtBot mentioned this pull request Nov 12, 2021

rpc: Only allow specific types to be P2(W)SH wrapped in decodescript #23486

Merged

mjdietzx force-pushed the refactor_ScriptToUniv branch from 570cbb7 to 9d8cd50 Compare November 13, 2021 15:34

mjdietzx added 5 commits November 13, 2021 11:21

refactor: merge ScriptPubKeyToUniv & ScriptToUniv into one function

b341711

refactor: prefer snake case, TxToUniv arg hashBlock renamed block_hash

6caf981

refactor: named args whenever ScriptToUniv or TxToUniv invoked

9d61af2

refactor: de-duplicate common help text for parsed destination address

63a657c

fuzz: speedup ScriptToUniv and TxToUniv tests by dropping redundant call

d4675f4

mjdietzx force-pushed the refactor_ScriptToUniv branch from 9d8cd50 to d4675f4 Compare November 13, 2021 20:11

mjdietzx force-pushed the refactor_ScriptToUniv branch from 886fac7 to d4675f4 Compare November 13, 2021 20:48

mjdietzx mentioned this pull request Nov 13, 2021

Refactor: Improve API design of ScriptToUniv, TxToUniv etc to return the UniValue instead of mutating a parameter/reference #23507

Closed

This was referenced Nov 18, 2021

scripted-diff: Use clang-tidy syntax for C++ named arguments #23545

Closed

scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) #23546

Merged

DrahtBot mentioned this pull request Nov 27, 2021

[WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags #23599

Closed

maflcko reviewed Dec 1, 2021

View reviewed changes

DrahtBot added the Needs rebase label Dec 1, 2021

maflcko added Up for grabs and removed Up for grabs labels Mar 22, 2022

fanquake mentioned this pull request Mar 25, 2022

refactor: followup of remove -deprecatedrpc=addresses flag #24673

Merged

mjdietzx closed this Mar 25, 2022

bitcoin locked and limited conversation to collaborators Mar 25, 2023

refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag #22924

refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag #22924

Uh oh!

Conversation

mjdietzx commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjdietzx commented Sep 14, 2021

Uh oh!

mjdietzx commented Sep 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

mjdietzx commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

laanwj Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

mjdietzx Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

mjdietzx Nov 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjdietzx Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

mjdietzx commented Nov 13, 2021

Uh oh!

maflcko Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

mjdietzx Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 1, 2021

Uh oh!

DrahtBot commented Mar 21, 2022

Uh oh!

maflcko commented Mar 22, 2022

Uh oh!

fanquake commented Mar 25, 2022

Uh oh!

mjdietzx commented Mar 25, 2022

Uh oh!

fanquake commented Mar 25, 2022

Uh oh!

Uh oh!

mjdietzx commented Sep 8, 2021 •

edited

Loading

DrahtBot commented Sep 29, 2021 •

edited

Loading

mjdietzx commented Oct 25, 2021 •

edited

Loading

mjdietzx Nov 13, 2021 •

edited

Loading