Skip to content

[wallet] Keypool topup cleanups#11044

Merged
laanwj merged 2 commits intobitcoin:masterfrom
jnewbery:keypool_topup_cleanups
Aug 18, 2017
Merged

[wallet] Keypool topup cleanups#11044
laanwj merged 2 commits intobitcoin:masterfrom
jnewbery:keypool_topup_cleanups

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Aug 14, 2017

A couple of minor cleanups suggested by @ryanofsky here: #11022 (review)

Does not affect functionality. Not required for v0.15.

@jnewbery jnewbery mentioned this pull request Aug 14, 2017
Merged
@maflcko maflcko added this to the 0.15.0 milestone Aug 14, 2017
@maflcko
Copy link
Member

maflcko commented Aug 14, 2017

Tagged for backport if there is an rc2, as this fixes a confusing warning when building from source.

@paveljanik
Copy link
Contributor

paveljanik commented Aug 14, 2017

Warnings removed.

ACK 61aa92d

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Aug 15, 2017

utACK 61aa92d9a854677cafefb15117c3d83d6bab6af6

@promag
Copy link
Contributor

promag commented Aug 16, 2017

Log message is consistent with others related.

Nit, I would reword 063aab4 to Remove unused CWallet::HasUnusedKeys since it doesn't really revert c25d90f, but keep the body.

ACK 61aa92d.

@sipa
Copy link
Member

sipa commented Aug 16, 2017

utACK 61aa92d9a854677cafefb15117c3d83d6bab6af6

@jnewbery
[wallet] Remove keypool_topup_cleanups
1221f60 
Unused function. Mostly reverts c25d90f

c25d90f... was merged as part of PR 11022 but is not required.
@jnewbery
[wallet] Add logging to MarkReserveKeysAsUsed
67ceff4
@jnewbery jnewbery force-pushed the keypool_topup_cleanups branch from 61aa92d to 67ceff4 Compare August 16, 2017 21:23
@jnewbery
Copy link
Contributor Author

jnewbery commented Aug 16, 2017

Thanks @promag . I've changed the commit message, but contents of the commit are the same. Check that 61aa92d == 67ceff4

laanwj
laanwj reviewed Aug 18, 2017
View reviewed changes
src/wallet/wallet.cpp
m_pool_key_to_index.erase(keypool.vchPubKey.GetID());
}
walletdb.ErasePool(index);
LogPrintf("keypool index %d removed\n", index);
Copy link
Member

@laanwj laanwj Aug 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to put this in the wallet debug category - it's just an info message, and not very useful for most end-users I guess? (it could even be vaguely worrying if you don't know what it's about)

Copy link
Contributor

@promag promag Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followup PR to change for all?

Copy link
Contributor Author

@jnewbery jnewbery Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches the existing 'keypool keep' logs, which are not wallet debug category.

This PR already has a bunch of ACKs and it's wanted for rc2, so I'd rather not change this now. I'll happily open a PR to make this (and other wallet logs) wallet debug category.

Copy link
Member

@laanwj laanwj Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok...

@promag
Copy link
Contributor

promag commented Aug 18, 2017

ACK 67ceff4.

@laanwj laanwj merged commit 67ceff4 into bitcoin:master Aug 18, 2017
laanwj added a commit that referenced this pull request Aug 18, 2017
@laanwj
Merge #11044: [wallet] Keypool topup cleanups
0e5b748 
67ceff4 [wallet] Add logging to MarkReserveKeysAsUsed (John Newbery)
1221f60 [wallet] Remove keypool_topup_cleanups (John Newbery)

Pull request description:

  A couple of minor cleanups suggested by @ryanofsky here: #11022 (review)

  Does not affect functionality. Not required for v0.15.

Tree-SHA512: d8d0698fd26ea49a4157e68669d5511095760c3a1ecfa3f917e3f273efbafb55c51a202d677614216eae3f796b6e8d17506b2ec2d4799a94f18981b396e65eec
laanwj pushed a commit that referenced this pull request Aug 21, 2017
@jnewbery @laanwj
[wallet] Remove keypool_topup_cleanups
4ef8374 
Unused function. Mostly reverts c25d90f

c25d90f... was merged as part of PR 11022 but is not required.

Github-Pull: #11044
Rebased-From: 1221f60
Tree-SHA512: da229b128bee5f124c009a1a2adfb4fa879366c81789824c426c9ce5209c835888a7e6cfeb1724551320a98cd08406a605372f84487a0d289cd6e02f9ac3ea21
laanwj pushed a commit that referenced this pull request Aug 21, 2017
@jnewbery @laanwj
[wallet] Add logging to MarkReserveKeysAsUsed
6ab120c 
Github-Pull: #11044
Rebased-From: 67ceff4
Tree-SHA512: 850c5b1010c84e164edf24a83ae36e46309b2eb7a67854bad509265ed590ba67d5f743a8416590da6ecca85fe4bda7f20e8c3152e422638eb7898db11a416af7
@jnewbery jnewbery deleted the keypool_topup_cleanups branch May 23, 2018 21:18
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 13, 2019
@laanwj
Merge #11044: [wallet] Keypool topup cleanups
08e2c02 
Summary:
67ceff4 [wallet] Add logging to MarkReserveKeysAsUsed (John Newbery)
1221f60 [wallet] Remove keypool_topup_cleanups (John Newbery)

Pull request description:

  A couple of minor cleanups suggested by @ryanofsky here: bitcoin/bitcoin#11022 (review)

  Does not affect functionality. Not required for v0.15.

Tree-SHA512: d8d0698fd26ea49a4157e68669d5511095760c3a1ecfa3f917e3f273efbafb55c51a202d677614216eae3f796b6e8d17506b2ec2d4799a94f18981b396e65eec

Backport of Core PR11044
bitcoin/bitcoin#11044

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3998
@UdjinM6 UdjinM6 mentioned this pull request Sep 19, 2019
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 20, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#11044: [wallet] Keypool topup cleanups
02328ae 
67ceff4 [wallet] Add logging to MarkReserveKeysAsUsed (John Newbery)
1221f60 [wallet] Remove keypool_topup_cleanups (John Newbery)

Pull request description:

  A couple of minor cleanups suggested by @ryanofsky here: bitcoin#11022 (review)

  Does not affect functionality. Not required for v0.15.

Tree-SHA512: d8d0698fd26ea49a4157e68669d5511095760c3a1ecfa3f917e3f273efbafb55c51a202d677614216eae3f796b6e8d17506b2ec2d4799a94f18981b396e65eec
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Docs Wallet

Projects

None yet

Milestone

0.15.0

Development

Successfully merging this pull request may close these issues.

7 participants

@jnewbery @maflcko @paveljanik @jonasschnelli @promag @sipa @laanwj