💬 davidgumberg commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2112533752)
Thanks for catching this, it's printed below in the added `LogDebug()` message but I forgot to increment this.
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2112533752)
Thanks for catching this, it's printed below in the added `LogDebug()` message but I forgot to increment this.
💬 davidgumberg commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#issuecomment-2917296094)
Rebased to address @w0xlt feedback.
(https://github.com/bitcoin/bitcoin/pull/32582#issuecomment-2917296094)
Rebased to address @w0xlt feedback.
🤔 w0xlt reviewed a pull request: "log: Additional compact block logging"
(https://github.com/bitcoin/bitcoin/pull/32582#pullrequestreview-2876111140)
ACK https://github.com/bitcoin/bitcoin/pull/32582/commits/2ff3a05fda11563fed303ef7a171f8edaeac111a
(https://github.com/bitcoin/bitcoin/pull/32582#pullrequestreview-2876111140)
ACK https://github.com/bitcoin/bitcoin/pull/32582/commits/2ff3a05fda11563fed303ef7a171f8edaeac111a
💬 instagibbs commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2112559081)
nit
```suggestion
unsigned int tx_requested_size = 0;
```
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2112559081)
nit
```suggestion
unsigned int tx_requested_size = 0;
```
👍 instagibbs approved a pull request: "log: Additional compact block logging"
(https://github.com/bitcoin/bitcoin/pull/32582#pullrequestreview-2876135402)
ACK 2ff3a05fda11563fed303ef7a171f8edaeac111a
(https://github.com/bitcoin/bitcoin/pull/32582#pullrequestreview-2876135402)
ACK 2ff3a05fda11563fed303ef7a171f8edaeac111a
📝 brunoerg opened a pull request: "wallet: sqlite: there is no need to have exclusive locking when mocking"
(https://github.com/bitcoin/bitcoin/pull/32632)
For in-memory SQLite databases, exclusive locking is generally not necessary because in-memory dbs are private to the connection that created them.
(https://github.com/bitcoin/bitcoin/pull/32632)
For in-memory SQLite databases, exclusive locking is generally not necessary because in-memory dbs are private to the connection that created them.
💬 vicjuma commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-2917340365)
Concept ACK
This separates viewing and spending logic and will be efficient especially if **exporting** it to an _easily_ **importable** format
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-2917340365)
Concept ACK
This separates viewing and spending logic and will be efficient especially if **exporting** it to an _easily_ **importable** format
📝 hebasto opened a pull request: "windows: Use predefined `RC_INVOKED` macro instead of custom one"
(https://github.com/bitcoin/bitcoin/pull/32633)
See: https://learn.microsoft.com/en-us/windows/win32/menurc/predefined-macros.
(https://github.com/bitcoin/bitcoin/pull/32633)
See: https://learn.microsoft.com/en-us/windows/win32/menurc/predefined-macros.
💬 strmfos commented on pull request "Replace dead gnome link notificator.cpp":
(https://github.com/bitcoin/bitcoin/pull/32629#discussion_r2112585490)
Agreed, the link doesn't directly help understand the code
Should I remove it?
(https://github.com/bitcoin/bitcoin/pull/32629#discussion_r2112585490)
Agreed, the link doesn't directly help understand the code
Should I remove it?
💬 enirox001 commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2917377896)
Concept ACK and Code Review ACK.
I think the PR's increased timeout tolerance for addnode peers is valuable. Given my limited domain knowledge, I'd suggest test coverage on potential silent stalls to ensure blocks are efficiently re-requested
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2917377896)
Concept ACK and Code Review ACK.
I think the PR's increased timeout tolerance for addnode peers is valuable. Given my limited domain knowledge, I'd suggest test coverage on potential silent stalls to ensure blocks are efficiently re-requested
📝 hebasto opened a pull request: "build: Add resource file and manifest to `bitcoin.exe`"
(https://github.com/bitcoin/bitcoin/pull/32634)
This PR is a follow up to https://github.com/bitcoin/bitcoin/pull/31375, which:
1. Adds a resource file for `bitcoin.exe` for consistency with other Windows executables.
2. Adds an application manifest to `bitcoin.exe`, which has been required for release binaries since https://github.com/bitcoin/bitcoin/pull/32396.
(https://github.com/bitcoin/bitcoin/pull/32634)
This PR is a follow up to https://github.com/bitcoin/bitcoin/pull/31375, which:
1. Adds a resource file for `bitcoin.exe` for consistency with other Windows executables.
2. Adds an application manifest to `bitcoin.exe`, which has been required for release binaries since https://github.com/bitcoin/bitcoin/pull/32396.
💬 hebasto commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2917389253)
> There seems a silent conflict with #32396.
Addressed in https://github.com/bitcoin/bitcoin/pull/32634.
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2917389253)
> There seems a silent conflict with #32396.
Addressed in https://github.com/bitcoin/bitcoin/pull/32634.
💬 achow101 commented on pull request "test: fix sync function in rpc_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/32630#issuecomment-2917407472)
ACK 4df4df45d7bc2e8be99325d40cda936aab87c083
(https://github.com/bitcoin/bitcoin/pull/32630#issuecomment-2917407472)
ACK 4df4df45d7bc2e8be99325d40cda936aab87c083
💬 achow101 commented on pull request "walletdb: Log additional exception error messages for corrupted wallets":
(https://github.com/bitcoin/bitcoin/pull/32598#discussion_r2112619227)
There is no need to log the exception type, especially since `runtime_error` is extraordinarily generic.
(https://github.com/bitcoin/bitcoin/pull/32598#discussion_r2112619227)
There is no need to log the exception type, especially since `runtime_error` is extraordinarily generic.
👍 hodlinator approved a pull request: "windows: Use predefined `RC_INVOKED` macro instead of custom one"
(https://github.com/bitcoin/bitcoin/pull/32633#pullrequestreview-2876232435)
crACK b97fc15df593d42296015218db197f377a71461c
The custom `WINDRES_PREPROC` #define was only read in clientversion.h. The built-in `RC_INVOKED` #define in the resource compiler is more straight forward to use to skip over sections of files that compiler does not support.
(https://github.com/bitcoin/bitcoin/pull/32633#pullrequestreview-2876232435)
crACK b97fc15df593d42296015218db197f377a71461c
The custom `WINDRES_PREPROC` #define was only read in clientversion.h. The built-in `RC_INVOKED` #define in the resource compiler is more straight forward to use to skip over sections of files that compiler does not support.
🤔 pablomartin4btc reviewed a pull request: "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet"
(https://github.com/bitcoin/bitcoin/pull/31423#pullrequestreview-2876222607)
code review ACK
Checked all pushes diffs. I like the latest/ current version with the refactoring & dedup re-loading wallets code. Since you touched that area, and there's no test for solvables wallet when there's no main/ local, perhaps you could add one?
I'll it test in a bit.
(https://github.com/bitcoin/bitcoin/pull/31423#pullrequestreview-2876222607)
code review ACK
Checked all pushes diffs. I like the latest/ current version with the refactoring & dedup re-loading wallets code. Since you touched that area, and there's no test for solvables wallet when there's no main/ local, perhaps you could add one?
I'll it test in a bit.
💬 pablomartin4btc commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2112616684)
nit: only if you to re-touch
```suggestion
# Always verify the backup path exists after migration
```
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2112616684)
nit: only if you to re-touch
```suggestion
# Always verify the backup path exists after migration
```
💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2917430246)
Rebased for silent merge conflict
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2917430246)
Rebased for silent merge conflict
🚀 achow101 merged a pull request: "test: fix sync function in rpc_psbt.py"
(https://github.com/bitcoin/bitcoin/pull/32630)
(https://github.com/bitcoin/bitcoin/pull/32630)
💬 achow101 commented on pull request "wallet: sqlite: there is no need to have exclusive locking when mocking":
(https://github.com/bitcoin/bitcoin/pull/32632#issuecomment-2917435899)
Does this materially change anything for performance? If not, I'd rather keep it as is for simplicity.
(https://github.com/bitcoin/bitcoin/pull/32632#issuecomment-2917435899)
Does this materially change anything for performance? If not, I'd rather keep it as is for simplicity.