wallet: Add tracing for sqlite statements #27801
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Concept ACK |
|
ACK f45e066 I tested by switching to this commit and doing the following In different window Then you can observe the bitcoind logs |
There was a problem hiding this comment.
tACK f45e066
Tested on OpenBSD 7.3, sqlite 3.41.0. Started bitcoind with debug=walletdb -loglevel=walletdb:trace, ran RPCs createwallet, getnewaddress and setlabel and observed the debug output emiting sqlite statements (SQLite Statement: INSERT or REPLACE into main values(.....).
|
Turning this on will log private keys as they are being written to the db, which seems a little scary. |
I found sqlite tracing was useful for debugging a test in bitcoin#27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options.
Yes when the tracing was turned on, having access to the debug log file would be like having access to the database file. The keys would at least be encrypted if the wallet had a password set, but it could still be scary/unexpected behavior. I looked into ways of trying to mask values in the sql statement, but the So for now I just updated the PR to avoid expanding any update statement that could contain sensitive values. We might be able to do something better in the future like add a more sensitive tracing level, or expand sql statements more intelligently, but the update should be a safe enough compromise that avoids leaking information while still making it easy to see full values by modifying the code. Updated f45e066 -> ff9d961 ( |
|
ACK ff9d961 |
|
ACK ff9d961 |
ff9d961 wallet: Add tracing for sqlite statements (Ryan Ofsky) Pull request description: I found sqlite tracing was useful for debugging a test in bitcoin#27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options. ACKs for top commit: achow101: ACK ff9d961 kevkevinpal: ACK bitcoin@ff9d961 theStack: ACK ff9d961 Tree-SHA512: 592fabfab3218cec36c2d00a21cd535fa840daa126ee8440c384952fbb3913180aa3796066c630087e933d6517f19089b867f158e0b737f25283a14799eefb05
ff9d961 wallet: Add tracing for sqlite statements (Ryan Ofsky) Pull request description: I found sqlite tracing was useful for debugging a test in bitcoin#27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options. ACKs for top commit: achow101: ACK ff9d961 kevkevinpal: ACK bitcoin@ff9d961 theStack: ACK ff9d961 Tree-SHA512: 592fabfab3218cec36c2d00a21cd535fa840daa126ee8440c384952fbb3913180aa3796066c630087e933d6517f19089b867f158e0b737f25283a14799eefb05
This reverts commit 5738ed1.
I found sqlite tracing was useful for debugging a test in #27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with
-debug=walletdb -loglevel=walletdb:traceoptions.