Remove redundant pre-TopUpKeypool check #16361

instagibbs commented at 2:13 PM on July 9, 2019: member

TopUpKeypool already has a quick check for IsLocked()

fanquake added the label RPC/REST/ZMQ on Jul 9, 2019

fanquake added the label Wallet on Jul 9, 2019

instagibbs force-pushed on Jul 9, 2019

MarcoFalke added the label Refactoring on Jul 9, 2019

MarcoFalke commented at 2:57 PM on July 9, 2019: member

ACK d40a642c14e4533d74ccd41daae6bcc0734501e0, but could fix the coding style in the wallet method, so that the commit can be reviewed by only looking at the diff:

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 097b92a262..084ec4599d 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3423,8 +3423,7 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
     {
         LOCK(cs_wallet);
 
-        if (IsLocked())
-            return false;
+        if (IsLocked()) return false;
 
         // Top up key pool
         unsigned int nTargetSize;

instagibbs force-pushed on Jul 9, 2019

instagibbs commented at 3:03 PM on July 9, 2019: member

good call @MarcoFalke , done

MarcoFalke commented at 3:13 PM on July 9, 2019: member

ACK d75d64b747c1cfca1e203755bdef50c017536bec, only looked at the diff

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash dfcc5bf7dc7fdef37456ef6b7a38e94248b574e921682dbc041268e79e939ed4 -

</details>

DrahtBot commented at 4:35 PM on July 9, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)

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.

DrahtBot added the label Needs rebase on Jul 10, 2019

DrahtBot commented at 11:31 AM on July 10, 2019: member

Needs rebase

instagibbs force-pushed on Jul 10, 2019

instagibbs renamed this:
~~Remove redundant pre-TopUpKeypool checks~~
Remove redundant pre-TopUpKeypool check
on Jul 10, 2019

instagibbs commented at 1:20 PM on July 10, 2019: member

Down to one check change due to s/key/destination/ refactoring merged in master.

fanquake removed the label Needs rebase on Jul 10, 2019

MarcoFalke commented at 1:28 PM on July 10, 2019: member

Down to one check change due to s/key/destination/ refactoring merged in master.

Are you sure? I think they just have been moved elsewhere.

Remove redundant pre-TopUpKeypool checks 96b6dd468a

instagibbs force-pushed on Jul 10, 2019

instagibbs commented at 1:40 PM on July 10, 2019: member

@MarcoFalke you'd think since I reviewed the responsible PR I would have known, but no I was wrong :) updated.

achow101 commented at 3:31 PM on July 10, 2019: member

ACK 96b6dd468a4cb6077d1a2267d620d99d39aac7d0 Reviewed the diff and checked that the if (!IsLocked()) TopUpKeypool() pattern is changed everywhere.

MarcoFalke referenced this in commit ff0aad8a40 on Jul 10, 2019

MarcoFalke merged this on Jul 10, 2019

MarcoFalke closed this on Jul 10, 2019

Munkybooty referenced this in commit fd890ed8ab on Nov 4, 2021

Munkybooty referenced this in commit 8c9b049e06 on Nov 6, 2021

Munkybooty referenced this in commit 5f77854558 on Nov 12, 2021

Munkybooty referenced this in commit c6a0b75971 on Nov 16, 2021

Munkybooty referenced this in commit a22eb0938b on Nov 18, 2021

Munkybooty referenced this in commit 21dd6ec656 on Nov 24, 2021

Munkybooty referenced this in commit 256b790a9a on Nov 30, 2021

Munkybooty referenced this in commit 0feb04ab6f on Nov 30, 2021

MarcoFalke locked this on Dec 16, 2021