/bitcoin

[wallet] fix zapwallettxes interaction with persistent mempool #10330

Merged
merged 3 commits into from Jul 17, 2017
+57 −55

Conversation

Reviewers

@TheBlueMatt TheBlueMatt

Assignees
No one assigned
Labels
Tests Wallet
Projects
None yet
Milestone
  0.15.0
7 participants
@jnewbery @jonasschnelli @MarcoFalke @sipa @achow101 @morcos @TheBlueMatt
@jnewbery
Member

jnewbery commented May 3, 2017

zapwallettxes previously did not interact well with persistent mempool.
zapwallettxes would cause wallet transactions to be zapped, but they
would then be reloaded from the mempool on startup. This commit softsets
persistmempool to false if zapwallettxes is enabled so transactions are
actually zapped.

This PR also fixes the zapwallettxes.py functional test, which did not properly test this feature. The test line:

     assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3])
     #there must be a expection because the unconfirmed wallettx0 must be gone by now

is not actually testing the presence of the transaction since the RPC is being called incorrectly (with an array instead of a string). The assert_raises() passes since an assert is raised, but it's not the one the test writer had in mind!

Fixes #9710 .

jnewbery referenced this pull request May 3, 2017

Closed

How should -zapwallettxes interact with persistent mempool? #9710

jonasschnelli added the Wallet label May 4, 2017

@jonasschnelli
Member

jonasschnelli commented May 4, 2017

utACK cfc234b.
@jnewbery
Member

jnewbery commented Jun 6, 2017

rebased
@jnewbery
Member

jnewbery commented Jun 18, 2017

Long-term, I think zapwallettxes should be a function in a standalone wallet tool (#8745). That's probably some time off, and in the mean time this is an improvement in behaviour. Any feedback on concept or implementation welcomed!
@MarcoFalke
Member

MarcoFalke commented Jun 18, 2017

utACK 15e72f5

MarcoFalke added this to the 0.15.0 milestone Jun 20, 2017

@sipa
Owner

sipa commented Jun 20, 2017

utACK 15e72f5. I haven't reviewed the test changes.
@MarcoFalke
Member

MarcoFalke commented Jun 20, 2017

Let's assume persistmempool is set in bitcoin.conf. Should there be an InitWarning when the user tries to setzapwallettxes?
@jnewbery
Member

jnewbery commented Jun 27, 2017

Let's assume persistmempool is set in bitcoin.conf. Should there be an InitWarning when the user tries to setzapwallettxes?

Perhaps, although this is an unlikely scenario since:

  • persistmempool is a new in 0.15, so no-one can be using it yet
  • the default value for persistmempool is true, so there's no reason to set it in bitcoin.conf

I'm inclined not to add the warning, but let me know if you feel strongly about it, and I can look at adding it.
@jnewbery
Member

jnewbery commented Jun 27, 2017

rebased

jnewbery changed the title from fix zapwallettxes interaction with persistent mempool to [wallet] fix zapwallettxes interaction with persistent mempool Jun 30, 2017

@MarcoFalke
Member

MarcoFalke commented Jul 2, 2017

Needs rebase
@jnewbery
Member

jnewbery commented Jul 2, 2017

rebased

This was referenced Jul 6, 2017

Open
Closed
@achow101
Contributor

achow101 commented Jul 12, 2017

utACK 46d9a4b
@morcos
Contributor

morcos commented Jul 13, 2017

utACK 46d9a4b
@TheBlueMatt

TheBlueMatt reviewed Jul 14, 2017
View changes

Didnt look too closely at the tests, but they look sane. Aside from the strange log message, utACK 46d9a4b

src/wallet/wallet.cpp
@@ -4010,6 +4010,11 @@ bool CWallet::ParameterInteraction()
LogPrintf("%s: parameter interaction: -salvagewallet=1 -> setting -rescan=1\n", __func__);
}
+ // -zapwallettx implies dropping the mempool on startup
+ if (GetBoolArg("-zapwallettxes", false) && SoftSetBoolArg("-persistmempool", false)) {
@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

"=<mode>" seems like a super strange string when <mode> isnt replaced out in the logs (plus its a boolean, what does <mode> mean?). zapwallettxes looks like the only one where it looks like this.

@jnewbery

jnewbery Jul 14, 2017

Member

Yes, you're right - that is strange. I just copied the existing log below.

I've fixed them both so they actually print the value of zapwallettxes.

Oh, and surprise! zapwallettxes isn't a bool!

@sipa
Owner

sipa commented Jul 15, 2017

Needs rebase.

jnewbery added some commits May 3, 2017

@jnewbery jnewbery [tests] fix flake8 warnings in zapwallettxes.py ff7365e
@jnewbery jnewbery [wallet] fix zapwallettxes interaction with persistent mempool 
zapwallettxes previously did not interact well with persistent mempool.
zapwallettxes would cause wallet transactions to be zapped, but they
would then be reloaded from the mempool on startup. This commit softsets
persistmempool to false if zapwallettxes is enabled so transactions are
actually zapped.
 e7a2181
@jnewbery jnewbery [logs] fix zapwallettxes startup logs
4c3b538
@jnewbery
Member

jnewbery commented Jul 15, 2017

rebased
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 16, 2017

utACK 4c3b538
@MarcoFalke
Member

MarcoFalke commented Jul 16, 2017

re-utACK 4c3b538

MarcoFalke added the Tests label Jul 17, 2017

@MarcoFalke MarcoFalke merged commit 4c3b538 into bitcoin:master Jul 17, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke added a commit that referenced this pull request Jul 17, 2017

@MarcoFalke MarcoFalke Merge #10330: [wallet] fix zapwallettxes interaction with persistent … 
…mempool


4c3b538 [logs] fix zapwallettxes startup logs (John Newbery)
e7a2181 [wallet] fix zapwallettxes interaction with persistent mempool (John Newbery)
ff7365e [tests] fix flake8 warnings in zapwallettxes.py (John Newbery)

Pull request description:

  zapwallettxes previously did not interact well with persistent mempool.
  zapwallettxes would cause wallet transactions to be zapped, but they
  would then be reloaded from the mempool on startup. This commit softsets
  persistmempool to false if zapwallettxes is enabled so transactions are
  actually zapped.

  This PR also Fixes the zapwallettxes.py functional test, which did not properly test this feature. The test line:

  ```py
       assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3])
       #there must be a expection because the unconfirmed wallettx0 must be gone by now
  ```
  is not actually testing the presence of the transaction since the RPC is being called incorrectly (with an array instead of a string). The `assert_raises()` passes since an assert is raised, but it's not the one the test writer had in mind!

  Fixes #9710 .

Tree-SHA512: e3236efc7a2fd2b3bf1d9e2e8a7726d470c57f5d95cf41b7bde264edc8817bd36a6f3feff52f8de8db0ef64b7247c88b24e7ff7cefaa706cba86fe4e2135a508
This commit was signed with a verified signature.
@MarcoFalke MarcoFalke
GPG key ID: D2EA4850E7528B25 Learn about signing commits
bf0a08b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment