Bugfix: RPC/Wallet: removeprunedfunds should flush the wallet db #8687

pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:bugfix_removeprunedfunds changing 3 files +18 −9

luke-jr commented at 8:55 AM on September 9, 2016: member

… rather than try to turn into the regular-interval wallet flushing thread

in src/wallet/rpcdump.cpp:None in 4d702d2a2d outdated

 349 | @@ -350,7 +350,8 @@ UniValue removeprunedfunds(const UniValue& params, bool fHelp)
 350 |          throw JSONRPCError(RPC_INTERNAL_ERROR, "Transaction does not exist in wallet.");
 351 |      }
 352 |  
 353 | -    ThreadFlushWalletDB(pwalletMain->strWalletFile);
 354 | +    LOCK(bitdb.cs_db);

MarcoFalke commented at 9:17 AM on September 9, 2016:

Shouldn't LOCKs be scoped?

luke-jr commented at 10:04 AM on September 9, 2016:

Yes, but the scope here is acceptable IMO? All it does is return a const.

in src/wallet/walletdb.cpp:None in 4d702d2a2d outdated
```
 917 | -
 918 | -                        // Flush wallet file so it's self contained
 919 | -                        bitdb.CloseDb(strFile);
 920 | -                        bitdb.CheckpointLSN(strFile);
 921 | -
 922 |                          bitdb.mapFileUseCount.erase(mi++);
```
MarcoFalke commented at 9:22 AM on September 9, 2016:

This causes the merge conflict

luke-jr commented at 10:03 AM on September 9, 2016:

Does it actually conflict if you try to merge it?

luke-jr commented at 9:15 PM on September 9, 2016:

Nevermind, I see my local master branch was just outdated.
MarcoFalke commented at 9:24 AM on September 9, 2016: member

utACK 4d702d2a2d301d67a560b97d0a2b85e4d07a98c0
MarcoFalke added the label Wallet on Sep 9, 2016
luke-jr force-pushed on Sep 9, 2016
Bugfix: RPC/Wallet: removeprunedfunds should flush the wallet db, rather than try to turn into the regular-interval wallet flushing thread 4197121ada
luke-jr force-pushed on Sep 9, 2016
luke-jr commented at 9:30 PM on September 9, 2016: member

Rebased, no changes.
luke-jr commented at 1:25 AM on September 10, 2016: member

(Would appreciate @instagibbs to confirm I got the intent right here.)
instagibbs commented at 1:32 AM on September 10, 2016: member

@luke-jr eh, I mostly aped similar functions to be honest, I wouldn't be an expert at db flushing.
luke-jr commented at 1:35 AM on September 10, 2016: member

@instagibbs Well, what was the purpose you had in mind for that line? Was it intended to flush the db? (all it actually did was rename the RPC thread)
instagibbs commented at 1:37 AM on September 10, 2016: member

Oh, yes

On Sep 9, 2016 9:35 PM, "Luke-Jr" notifications@github.com wrote:

@instagibbs https://github.com/instagibbs Well, what was the purpose you had in mind for that line? Was it intended to flush the db? (all it actually did was rename the RPC thread)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub #8687 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC03rbwwoAaZKBE87TXZbgy_ZDaT9xks5qogl9gaJpZM4J41h4 .
laanwj commented at 3:41 PM on September 20, 2016: member
This is indeed wrong (see discussion in #8765) but I don't think this is the right solution:
- The ThreadFlushDB is misnamed, it is not a flushing thread but a compaction/consolidation thread.
- There is no need to trigger a db compaction at this point. A normal cwallet->Flush() would do, if you want an explicit flush here.
luke-jr commented at 8:52 PM on September 20, 2016: member

Depends on the goal, I guess. I assumed since this is removing data, compacting might be desirable to ensure it gets removed entirely.
laanwj commented at 5:54 AM on September 21, 2016: member

Depends on the goal, I guess. I assumed since this is removing data, compacting might be desirable to ensure it gets removed entirely.

That's a good point in itself, however this is a very slow operation with a large wallet, so doing it after every command is non-optimal.

Also, although I called it 'compaction' it does not have any guarantee of getting rid of old data. It just consolidates the log files into wallet.dat. There is however a call to do a rewriting of the wallet, which will have the effect of getting rid of all unused blocks: CDB::Rewrite. Which we use when encrypting, but the drawback is that it requires a full restart of the client. Maybe adding an explicit RPC call to do that operation at some point in the future (when we have more calls to remove, say, privacy-sensitive metadata) makes sense. I don't know.

But not automatically, not in this place.

(OTOH this is an excellent idea to add to @jonasschnelli's new wallet tool in #8745)
laanwj commented at 6:12 AM on September 21, 2016: member

Closing in favor of #8765
laanwj closed this on Sep 21, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#8765 [Wallet] remove "unused" ThreadFlushWalletDB from removeprunedfunds