Improvement on importprivkey method. Errors can be triggered early.
rpc: trigger error before dealing strSecret / strLabel #22549
pull bdescamps wants to merge 1 commits into bitcoin:master from bdescamps:importprivkey_optimize changing 1 files +5 −5-
bdescamps commented at 7:35 PM on July 25, 2021: none
-
trigger error before dealing strSecret / strLabel 35ac5abf1c
- ccr781804 approved
-
ccr781804 commented at 8:04 PM on July 25, 2021: none
For me. It's very difficult to understand clearly.
- DrahtBot added the label RPC/REST/ZMQ on Jul 25, 2021
- DrahtBot added the label Wallet on Jul 25, 2021
-
bdescamps commented at 9:02 PM on July 25, 2021: none
I think it's better to deal directly exit function. Here we can trigger two JSONRPCError, without instantiate string for strLabel & strSecret
-
laanwj commented at 2:03 PM on July 28, 2021: member
What does this change from a user perspective?
-
bdescamps commented at 2:18 PM on July 28, 2021: none
At this time nothing, but it can prevent future bug.
-
laanwj commented at 2:40 PM on July 28, 2021: member
What kind of bug?
-
bdescamps commented at 3:09 PM on July 28, 2021: none
A bug create by a bad interpretation of code Here we create 2 variables strSecret StrLabel.
- strSecret which is instanciate by a request params
- strLabel which is instanciate by an empty string and set after by a request params (if it is present)
There is no reason to dealing them now because we don't use them. Moreover if the wallet rescan is disabled or if the wallet is currently rescanning we have creating all that for nothing
-
tryphe commented at 9:15 AM on July 30, 2021: contributor
This code doesn't get called very much (people only use it to manually type in their legacy private keys) and the performance doesn't really matter. It would save us from copying a string each time an error happens...
The initializations could also be moved up before
LOCK(pwallet->cs_wallet);to save on lock time. The initialization ofstrLabelcould be moved down further also, to line 168.:bike: :bike: :bike:
-
MarcoFalke commented at 11:38 AM on August 2, 2021: member
With argument parsing moving to RPCHelpMan, this will become obsolete. So tend to NACK on this diff.
Maybe it could make sense to use the zeroing-on-destruction string here.
-
laanwj commented at 2:55 PM on August 18, 2021: member
As it stands, this doesn't seem worth the hassle, so closing this.
- laanwj closed this on Aug 18, 2021
- DrahtBot locked this on Aug 18, 2022
Contributors
Labels