Skip to content

[QT] Don't ask for a passphrase to getnewaddress.#2904

Merged
gmaxwell merged 2 commits into
bitcoin:masterfrom
gmaxwell:newaddr-no-passphrase
Aug 28, 2013
Merged

[QT] Don't ask for a passphrase to getnewaddress.#2904
gmaxwell merged 2 commits into
bitcoin:masterfrom
gmaxwell:newaddr-no-passphrase

Conversation

@gmaxwell
Copy link
Copy Markdown
Contributor

With an encrypted wallet the GUI was prompting for a passphrase every time
the user requested a new address. This is unnecessary, increases the
exposure to keyboard sniffers, and discourages using fresh addresses for
every transaction.

Instead only prompt for a passphrase when the keypool runs out, also call
the new address function with the flag that prevents reuse.

@gmaxwell
Copy link
Copy Markdown
Contributor Author

So, the design choices here were to reuse or prompt when the keypool ran dry. Reuse has the advantage that the behavior would never change even when the pool empties, but it would reuse a previously assigned address, which is really surprising. There would also be an option to reuse after prompting if the wallet didn't unlock.

Codeshark, said prompt on empty. I agree. If someone wants to reuse when they don't have the key— the list is there.

I tested this by starting up with an encrypted wallet and hitting new address until the keypool ran out and it promoted me. I dismissed the dialog and retried a few times before finally letting it unlock and confirming it worked as expected.

@BitcoinPullTester
Copy link
Copy Markdown

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/0eb506a01a32cc78609ee9071b0d5302488496ec for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@gmaxwell
Copy link
Copy Markdown
Contributor Author

@wumpus @Diapolo Any comments? I don't normally change GUI code.

@Diapolo
Copy link
Copy Markdown

Diapolo commented Aug 17, 2013

I took a quick look at GetKeyFromPool() and AFAIK we want to get rid of that default address/kay anyway, right? That pull could also remove the fAllowReuse parameter from that function, as it's false after this pull everywhere in the code.

Code looks good and does what it sais, no need to enter the passphrase when requesting a new receiving address.

@gmaxwell
Copy link
Copy Markdown
Contributor Author

@Diapolo Good observation on default, I hadn't thought to check to find out if that were the last user of it.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Aug 22, 2013

Good change.

Agree with @Diapolo with regard to default key and allow reuse.

@Diapolo
Copy link
Copy Markdown

Diapolo commented Aug 23, 2013

AFAIK GetKeyFromPool() is now true at least once after paymentrequest pull was merged. Can you guys have a look into if this makes sense.

With an encrypted wallet the GUI was prompting for a passphrase every time
 the user requested a new address.  This is unnecessary, increases the
 exposure to keyboard sniffers, and discourages using fresh addresses for
 every transaction.

Instead only prompt for a passphrase when the keypool runs out, also call
 the new address function with the flag that prevents reuse.

Thanks to AlexNagy on IRC for pointing this out and who wouldn't take any
 lip from a curmudgeonly developer and insisted on what he knew to be true.
With the GUI password fix this was always false.
@gmaxwell
Copy link
Copy Markdown
Contributor Author

fAllowReuse gone and rebased post paymentrequests so I could remove the flag there too.

@BitcoinPullTester
Copy link
Copy Markdown

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/71ac5052d83fcba21a09e5e2b7ad66faea6bd42a for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Aug 24, 2013

ACK

@sipa
Copy link
Copy Markdown
Member

sipa commented Aug 25, 2013

ACK core changes.

gmaxwell added a commit that referenced this pull request Aug 28, 2013
[QT] Don't ask for a passphrase to getnewaddress.
@gmaxwell gmaxwell merged commit 1ef0067 into bitcoin:master Aug 28, 2013
IntegralTeam pushed a commit to IntegralTeam/bitcoin that referenced this pull request Jun 4, 2019
…#2904)

* Move code to write archived ISLOCKs into its own method

We'll need this from another method as well later.

* Return ISLOCK instead of conflicting txid in GetConflictingTx/GetConflictingLock

* Implement GetInstantSendLocksByParent and RemoveChainedInstantSendLocks

These allow to easily delete multiple chains (actually trees) of ISLOCKs
in one go.

* Implement RemoveConflictedTx and call it from RemoveMempoolConflictsForLock

Also add "retryChildren" parameter to RemoveNonLockedTx so that we can
skip retrying of non-locked children TXs.

* Properly handle/remove conflicted TXs (between mempool and new blocks)

* Track non-locked TXs by inputs

* Implement and call ResolveBlockConflicts

* Also call ResolveBlockConflicts from ConnectBlock

But only when a block is known to have a conflict and at the same time is
ChainLocked, which causes the ISLOCK to be pruned.

* Split out RemoveChainLockConflictingLock from ResolveBlockConflicts

* Implement "quorum getrecsig" RPC

* Include decoded TX data in result of create_raw_tx

* Implement support for CLSIG in mininode.py

* Fix condition for update of nonLockedTxs.pindexMined

* Only add entries to nonLockedTxsByInputs when AddNonLockedTx is called for the first time

* Implement support for ISLOCK in mininode.py

* Implement tests for ChainLock vs InstantSend lock conflict resolution

* Handle review comment

Bail out (continue) early
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants