💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199487534)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
What's the reason for using (0, 0) flags here and other places instead of (SER_DISK, CLIENT_VERSION) flags used elsewhere? Having a code comment about this somewhere would be very helpful for understanding. It seems a little dodgy to get rid of SER_DISK flag if present or future code might rely on it.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199487534)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
What's the reason for using (0, 0) flags here and other places instead of (SER_DISK, CLIENT_VERSION) flags used elsewhere? Having a code comment about this somewhere would be very helpful for understanding. It seems a little dodgy to get rid of SER_DISK flag if present or future code might rely on it.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198934920)
In commit "test: add coverage for db cursor prefix range iteration" (83b4e50291a0bd2c45f370cb18bb479d8f73bc71)
Setting this flag seems unnecessary and unrelated to the test
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198934920)
In commit "test: add coverage for db cursor prefix range iteration" (83b4e50291a0bd2c45f370cb18bb479d8f73bc71)
Setting this flag seems unnecessary and unrelated to the test
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198957038)
In commit "walletdb: Refactor crypted key loading to its own function" (59ac3bccffe3fda352dc068fa3a0b507ed9a73d6)
Similar to previous commit, now `wss.nCKeys` will not be incremented if this fails. Change in behavior could be noted in commit message.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198957038)
In commit "walletdb: Refactor crypted key loading to its own function" (59ac3bccffe3fda352dc068fa3a0b507ed9a73d6)
Similar to previous commit, now `wss.nCKeys` will not be incremented if this fails. Change in behavior could be noted in commit message.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199488667)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
Would be helpful to have a code comment here saying enum number values are significant because if there are different error codes from reading different rows of the database, the error code with the highest number is the one that will be returned.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199488667)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
Would be helpful to have a code comment here saying enum number values are significant because if there are different error codes from reading different rows of the database, the error code with the highest number is the one that will be returned.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199490724)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
It looks like in this commit, the side-effect in previous commit "walletdb: Refactor key reading and loading to its own function" (e048fe71038caddb49a5875727a14e05f6edb0d3) of changing the `wss.nKeys` value in case of key errors is partially reverted, because now the number of keys is incremented unconditionally, while the previous commit added more conditions.
Woul
...
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199490724)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
It looks like in this commit, the side-effect in previous commit "walletdb: Refactor key reading and loading to its own function" (e048fe71038caddb49a5875727a14e05f6edb0d3) of changing the `wss.nKeys` value in case of key errors is partially reverted, because now the number of keys is incremented unconditionally, while the previous commit added more conditions.
Woul
...
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199493269)
re: https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1102044608
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
> We print all corrupt records of one type, not all the corrupted records
I guess I disagree a little, but I think it is good to log errors about everything we know is corrupt, even if we don't log errors about things we can't know are corrupt.
Regardless, if there is a change in behavior
...
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1199493269)
re: https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1102044608
In commit "walletdb: Refactor legacy wallet record loading into its own function" (8648511b567dfcdea7ffa5ac4595a43a768f5525)
> We print all corrupt records of one type, not all the corrupted records
I guess I disagree a little, but I think it is good to log errors about everything we know is corrupt, even if we don't log errors about things we can't know are corrupt.
Regardless, if there is a change in behavior
...
💬 achow101 commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1555399958)
re-ACK 1bce12acd3e271a7c88d9400b4e3a5645bc8a911
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1555399958)
re-ACK 1bce12acd3e271a7c88d9400b4e3a5645bc8a911
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199572071)
> but is there a bug?
No, there is not. I've double-checked the code, it looks OK for me too.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199572071)
> but is there a bug?
No, there is not. I've double-checked the code, it looks OK for me too.
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1555785546)
Reviewing the code:
- [x] 974b4293a5f2fe01302c608bf0063e3b52721d0b "kernel: Add notification interface"
- [x] ec5043d8c120332c0bf916b5b576aff8374af1ed "kernel: Add headerTip method to notifications"
- [ ] 1be1c16b70edc5def98eb1903cee08b5d546ba88 "kernel: Add progress method to notifications"
- [ ] 483fe64956179872e5821483474c52d955949e40 "kernel: Add warning method to notifications"
- [ ] 3785d38417618e921c5233cf14aa3c802f37387b "refactor: Move ScheduleBatchPriority to its own file"
- [
...
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1555785546)
Reviewing the code:
- [x] 974b4293a5f2fe01302c608bf0063e3b52721d0b "kernel: Add notification interface"
- [x] ec5043d8c120332c0bf916b5b576aff8374af1ed "kernel: Add headerTip method to notifications"
- [ ] 1be1c16b70edc5def98eb1903cee08b5d546ba88 "kernel: Add progress method to notifications"
- [ ] 483fe64956179872e5821483474c52d955949e40 "kernel: Add warning method to notifications"
- [ ] 3785d38417618e921c5233cf14aa3c802f37387b "refactor: Move ScheduleBatchPriority to its own file"
- [
...
💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1555793887)
CI is happy now
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1555793887)
CI is happy now
💬 Sjors commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1555806652)
> > My understanding was that doing an optimal sort for a given cluster, requires it to have a few dozen transactions.
>
> It's the opposite really. [...] Up to 15-20 transactions we may be able to find it using an exponential-time algorithm; above that [...]
I meant to say "no more than a few dozen transactions".
So I guess we're ok with falling back to less than optimal sorting _within_ each cluster?
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1555806652)
> > My understanding was that doing an optimal sort for a given cluster, requires it to have a few dozen transactions.
>
> It's the opposite really. [...] Up to 15-20 transactions we may be able to find it using an exponential-time algorithm; above that [...]
I meant to say "no more than a few dozen transactions".
So I guess we're ok with falling back to less than optimal sorting _within_ each cluster?
⚠️ ArmchairCryptologist opened an issue: "Frequent "Timeout downloading block" with 24.1.0"
(https://github.com/bitcoin/bitcoin/issues/27705)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
After updating to 24.1.0 yesterday I'm seeing frequent "Timeout downloading block" on two of my four public nodes, which is pretty annoying since the timeout is 10 minutes, which means every single timeout causes a 10-minute stall on new block downloads. The other two public nodes both have a couple of timeouts, but nothing frequent. A private listen=0 tor-only node also running 24.1.0 doe
...
(https://github.com/bitcoin/bitcoin/issues/27705)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
After updating to 24.1.0 yesterday I'm seeing frequent "Timeout downloading block" on two of my four public nodes, which is pretty annoying since the timeout is 10 minutes, which means every single timeout causes a 10-minute stall on new block downloads. The other two public nodes both have a couple of timeouts, but nothing frequent. A private listen=0 tor-only node also running 24.1.0 doe
...
🤔 hebasto reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1435386964)
ACK d96c82a76775b1a41c098e6af60130fbdbba9975, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1435386964)
ACK d96c82a76775b1a41c098e6af60130fbdbba9975, I have reviewed the code and it looks OK.
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1555876922)
Updated d96c82a76775b1a41c098e6af60130fbdbba9975 -> a30cd548f1bc0ed73a4e157c6e81dbcf2542a916 ([chainstateRmKernelUiInterface_10](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_10) -> [chainstateRmKernelUiInterface_11](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_10..chainstateRmKernelUiInterface_11)).
* Addressed @hebasto's [comment](https:
...
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1555876922)
Updated d96c82a76775b1a41c098e6af60130fbdbba9975 -> a30cd548f1bc0ed73a4e157c6e81dbcf2542a916 ([chainstateRmKernelUiInterface_10](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_10) -> [chainstateRmKernelUiInterface_11](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_10..chainstateRmKernelUiInterface_11)).
* Addressed @hebasto's [comment](https:
...
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1555879559)
Rebased a30cd548f1bc0ed73a4e157c6e81dbcf2542a916 -> bbfbcd0360e486d47025a412f2c0f4e8535ab255 ([chainstateRmKernelUiInterface_11](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_11) -> [chainstateRmKernelUiInterface_12](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_12), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_11..chainstateRmKernelUiInterface_12)).
* Fixed broken include introduced by ht
...
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1555879559)
Rebased a30cd548f1bc0ed73a4e157c6e81dbcf2542a916 -> bbfbcd0360e486d47025a412f2c0f4e8535ab255 ([chainstateRmKernelUiInterface_11](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_11) -> [chainstateRmKernelUiInterface_12](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_12), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_11..chainstateRmKernelUiInterface_12)).
* Fixed broken include introduced by ht
...
💬 josibake commented on pull request "random: drop syscall wrapper usage for getrandom()":
(https://github.com/bitcoin/bitcoin/pull/27699#issuecomment-1555881591)
ACK https://github.com/bitcoin/bitcoin/pull/27699/commits/f33211a4fe563e03542343ee033d7f5ec7887cbf
Nice to be able to drop the work-around :rocket:
I got the same as you for the guix build:
```
1c981a6d7cce696f4b562443ef53f401dfa6edc4704154f453ee4f66b8fc72d6 aarch64-linux-gnu/bitcoin-f33211a4fe56-aarch64-linux-gnu-debug.tar.gz
c8d0b81e5ad5f504650690c9b0192338b22a09aeead9cc1a48054e228704f1a8 aarch64-linux-gnu/bitcoin-f33211a4fe56-aarch64-linux-gnu.tar.gz
4e655cf22f003455a14edd9408e
...
(https://github.com/bitcoin/bitcoin/pull/27699#issuecomment-1555881591)
ACK https://github.com/bitcoin/bitcoin/pull/27699/commits/f33211a4fe563e03542343ee033d7f5ec7887cbf
Nice to be able to drop the work-around :rocket:
I got the same as you for the guix build:
```
1c981a6d7cce696f4b562443ef53f401dfa6edc4704154f453ee4f66b8fc72d6 aarch64-linux-gnu/bitcoin-f33211a4fe56-aarch64-linux-gnu-debug.tar.gz
c8d0b81e5ad5f504650690c9b0192338b22a09aeead9cc1a48054e228704f1a8 aarch64-linux-gnu/bitcoin-f33211a4fe56-aarch64-linux-gnu.tar.gz
4e655cf22f003455a14edd9408e
...
🚀 fanquake merged a pull request: "doc: remove Security section from build-unix.md"
(https://github.com/bitcoin/bitcoin/pull/27688)
(https://github.com/bitcoin/bitcoin/pull/27688)
💬 fanquake commented on pull request "doc: filter out merge-script from list of authors":
(https://github.com/bitcoin/bitcoin/pull/27703#issuecomment-1555883473)
Going to cherry-pick this into a larger set of changes, to cleanup the release process documentation.
(https://github.com/bitcoin/bitcoin/pull/27703#issuecomment-1555883473)
Going to cherry-pick this into a larger set of changes, to cleanup the release process documentation.
✅ fanquake closed a pull request: "doc: filter out merge-script from list of authors"
(https://github.com/bitcoin/bitcoin/pull/27703)
(https://github.com/bitcoin/bitcoin/pull/27703)
💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1555885480)
reACK https://github.com/bitcoin/bitcoin/pull/25796/commits/1bce12acd3e271a7c88d9400b4e3a5645bc8a911
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1555885480)
reACK https://github.com/bitcoin/bitcoin/pull/25796/commits/1bce12acd3e271a7c88d9400b4e3a5645bc8a911