debug log number of unknown wallet records on load #12888

instagibbs commented at 12:48 AM on April 5, 2018: member

This would have saved me some time during wallet debugging, with minimal logging clutter.

fanquake added the label Wallet on Apr 5, 2018

fanquake added the label Utils/log/libs on Apr 5, 2018

Empact commented at 12:51 AM on April 5, 2018: member

How about logging the unknown record strTypes themselves, rather than the number? Would provide additional info and avoid the need for storing the state.

instagibbs commented at 1:00 AM on April 5, 2018: member

I've done a spammier approach on my own branches, but I suppose I could throw an std::set at the problem if people aren't against the idea.

in src/wallet/walletdb.cpp:603 in cf0a63a697 outdated

 599 | @@ -597,6 +600,7 @@ DBErrors CWalletDB::LoadWallet(CWallet* pwallet)
 600 |  
 601 |      LogPrintf("Keys: %u plaintext, %u encrypted, %u w/ metadata, %u total\n",
 602 |             wss.nKeys, wss.nCKeys, wss.nKeyMeta, wss.nKeys + wss.nCKeys);
 603 | +    LogPrintf("Unknown wallet records: %u\n", wss.m_unknown_records);

practicalswift commented at 6:01 AM on April 5, 2018:

To reduce the amount of logging I suggest printing this only if wss.m_unknown_records != 0.

Alternatively add wss.m_unknown_records printing to the already existing log entry (Keys: …) above.

laanwj commented at 8:03 AM on April 5, 2018:

To reduce the amount of logging I suggest printing this only if wss.m_unknown_records != 0.

Or print it on the same line as the Keys: (have one wallet statistics record). In that case it doesn't matter to log it every time.

practicalswift commented at 8:14 AM on April 5, 2018:

@laanwj Agreed! That was suggestion 2 :-)

promag commented at 8:44 AM on April 5, 2018:

👍 @practicalswift

instagibbs commented at 3:11 PM on April 5, 2018:

I split it out because the print line is "Keys:", but I suppose this could have meant "Key/Value" versus "crypto keys"?

instagibbs commented at 3:13 PM on April 5, 2018:

It's counting only the crypto keys as "total", not metadata which I think led me to believe this.

If I add it to the above line, I suppose I'll ommit it from "total"?

laanwj commented at 11:45 AM on April 8, 2018:

Indeed, I didn't mean to include it in keys but have it seperately e.g.

Keys: %u plaintext, %u encrypted, %u w/ metadata, %u total. Unknown wallet records: %u.

laanwj commented at 8:03 AM on April 5, 2018: member

ConceptACK

promag commented at 8:46 AM on April 5, 2018: member

Concept ACK. @instagibbs can you detail the purpose of this counter?

laanwj commented at 8:54 AM on April 5, 2018: member

If you want to log spammier stuff, put it in the "wallet" category. A total should suffice in the default case.

On Thu, Apr 5, 2018, 03:00 Gregory Sanders notifications@github.com wrote:

I've done a spammier approach on my own branches, but I suppose I could throw an std::set at the problem if people aren't against the idea.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/12888#issuecomment-378790484, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutvyxDHCK8bsujSlgy9c6AlBMJ8b4ks5tlWzAgaJpZM4THtIc .

instagibbs commented at 3:14 PM on April 5, 2018: member

@promag detail in the OP, or in the commit itself?

practicalswift commented at 8:13 PM on April 5, 2018: contributor

Agree with @laanwj after thinking about it: put this in the wallet category. Logging to the default log level should be reserved for things that are likely to be of interest for the end-user.

promag commented at 10:43 PM on April 5, 2018: member

@instagibbs I mean in what way is this counter useful? Just to understand why you are adding this.

instagibbs commented at 10:45 PM on April 5, 2018: member

@promag lets you know if there are "num" amount of records your wallet doesn't recognize. In my case it would have told me exactly what was going on(type typo).

instagibbs force-pushed on Apr 9, 2018

instagibbs commented at 2:57 PM on April 9, 2018: member

put on single log entry as requested by @laanwj

laanwj commented at 4:34 PM on April 9, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12888/commits/6b25bae87c03d54d9624d4cce3f76f0033df7459

practicalswift commented at 5:11 PM on April 9, 2018: contributor

utACK 6b25bae87c03d54d9624d4cce3f76f0033df7459

instagibbs commented at 5:40 PM on April 10, 2018: member

one test timed out, kicked it. Maybe it'll be faster after the unit test improvements...

laanwj commented at 6:33 AM on April 11, 2018: member

Noticed while testing: This should probably ignore bestblock and bestblock_nomerkle as it does write those.

debug log number of unknown wallet records on load 72ec5b7766

instagibbs force-pushed on Apr 11, 2018

instagibbs commented at 3:48 PM on April 11, 2018: member

updated to filter bestblock and bestblock_nomerkle entries.

laanwj commented at 7:16 AM on April 12, 2018: member

tested ack 72ec5b77668fcbd838dbd19723728af2c9a97208

laanwj merged this on Apr 12, 2018

laanwj closed this on Apr 12, 2018

laanwj referenced this in commit 39439e5ab4 on Apr 12, 2018

laanwj commented at 2:01 PM on April 13, 2018: member

Looks like we forgot to account for minversion here.

jasonbcox referenced this in commit ea8cef58c8 on Sep 27, 2019

PastaPastaPasta referenced this in commit fa99b086ec on Jun 10, 2020

PastaPastaPasta referenced this in commit fb983406cf on Jun 10, 2020

PastaPastaPasta referenced this in commit 3f24c879b2 on Jun 10, 2020

jonspock referenced this in commit ec8b6a7244 on Sep 30, 2020

jonspock referenced this in commit 7d15dae2b8 on Oct 5, 2020

jonspock referenced this in commit f2cdbfedba on Oct 10, 2020

PastaPastaPasta referenced this in commit a4195d9638 on Nov 9, 2020

PastaPastaPasta referenced this in commit d1f709b16c on Nov 12, 2020

PastaPastaPasta referenced this in commit 5d40de49f2 on Dec 11, 2020

DrahtBot locked this on Sep 8, 2021