✅ furszy closed a pull request: "wallet: bugfix, 'AvailableCoins' invalid output type set for raw PK outputs"
(https://github.com/bitcoin/bitcoin/pull/27478)
(https://github.com/bitcoin/bitcoin/pull/27478)
💬 fanquake commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1513082712)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1513082712)
Concept ACK
💬 MarcoFalke commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#discussion_r1170005753)
thx, done
(https://github.com/bitcoin/bitcoin/pull/27483#discussion_r1170005753)
thx, done
👍 stickies-v approved a pull request: "doc: remove outdated version number usage from release-process"
(https://github.com/bitcoin/bitcoin/pull/27484#pullrequestreview-1390113171)
ACK fde224a6610699a6a28cc27e299ac14cbf7e16ca
Couldn't find any other outdated examples.
(https://github.com/bitcoin/bitcoin/pull/27484#pullrequestreview-1390113171)
ACK fde224a6610699a6a28cc27e299ac14cbf7e16ca
Couldn't find any other outdated examples.
📝 fanquake opened a pull request: "[WIP] ci: standardize custom libc++ usage in MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27485)
Use `-isystem` & `-nostd*` flags, which is the preferred way to use a custom libc++ (ours is libc++ build with MSAN) with Clang, as opposed to our current ad-hoc flags.
See: https://releases.llvm.org/16.0.0/projects/libcxx/docs/UsingLibcxx.html#using-a-custom-built-libc for more info:
> Most compilers provide a way to disable the default behavior for finding the standard library and to override it with custom paths. With Clang, this can be done with:
```bash
clang++ -nostdinc++ -nostdlib++
...
(https://github.com/bitcoin/bitcoin/pull/27485)
Use `-isystem` & `-nostd*` flags, which is the preferred way to use a custom libc++ (ours is libc++ build with MSAN) with Clang, as opposed to our current ad-hoc flags.
See: https://releases.llvm.org/16.0.0/projects/libcxx/docs/UsingLibcxx.html#using-a-custom-built-libc for more info:
> Most compilers provide a way to disable the default behavior for finding the standard library and to override it with custom paths. With Clang, this can be done with:
```bash
clang++ -nostdinc++ -nostdlib++
...
💬 0xB10C commented on pull request "doc: remove outdated version number usage from release-process":
(https://github.com/bitcoin/bitcoin/pull/27484#issuecomment-1513170888)
ACK
(https://github.com/bitcoin/bitcoin/pull/27484#issuecomment-1513170888)
ACK
💬 jnewbery commented on pull request "p2p: Track orphans by who provided them":
(https://github.com/bitcoin/bitcoin/pull/26551#discussion_r1170060171)
> should we move orphan processing outside of ProcessMessages and SendMessages entirely, and just do it separately, independent of both who sent the orphan and who sent the parent?
>
> Unless someone has a good reason not to, I think I might draft that up as an alternative PR so the approaches can be compared more directly.
See #19364
(https://github.com/bitcoin/bitcoin/pull/26551#discussion_r1170060171)
> should we move orphan processing outside of ProcessMessages and SendMessages entirely, and just do it separately, independent of both who sent the orphan and who sent the parent?
>
> Unless someone has a good reason not to, I think I might draft that up as an alternative PR so the approaches can be compared more directly.
See #19364
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1170062170)
Mainly to be consistent with the chain interface, wallet and spkm sources. Timestamps are represented as `int64_t` there. E.g. the `FoundBlock` class, `findFirstBlockWithTimeAndHeight` method, the `CWallet::m_best_block_time` member, first key time, descriptor creation time etc.
I think that is clear that block times are always in seconds, so the use of an arithmetic type shouldn't be much of a worry but we could switch all of them to `NodeSeconds` too (probably in another PR that does it all
...
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1170062170)
Mainly to be consistent with the chain interface, wallet and spkm sources. Timestamps are represented as `int64_t` there. E.g. the `FoundBlock` class, `findFirstBlockWithTimeAndHeight` method, the `CWallet::m_best_block_time` member, first key time, descriptor creation time etc.
I think that is clear that block times are always in seconds, so the use of an arithmetic type shouldn't be much of a worry but we could switch all of them to `NodeSeconds` too (probably in another PR that does it all
...
👍 stickies-v approved a pull request: "[24.x] Additional backports for 24.1"
(https://github.com/bitcoin/bitcoin/pull/27474#pullrequestreview-1390158761)
ACK dc711fbd32653b09e196f72942106114a32353f4
Verified that the backports correspond to their original PR, and I can't see any silent merge conflicts. Release notes match the backported changes.
(https://github.com/bitcoin/bitcoin/pull/27474#pullrequestreview-1390158761)
ACK dc711fbd32653b09e196f72942106114a32353f4
Verified that the backports correspond to their original PR, and I can't see any silent merge conflicts. Release notes match the backported changes.
💬 rebroad commented on issue "add ability to remove (dust) UTXOs":
(https://github.com/bitcoin/bitcoin/issues/23875#issuecomment-1513184940)
I do think there's an unforeseen cost in storing these dust UTXOs, which might result in people willing to pay into a pot to have them cleared up - a bit like people grouping together to clean up the beaches of litter. If people didn't have to pay to pick-up their own dust, then it dust-picking might occur more often.
(https://github.com/bitcoin/bitcoin/issues/23875#issuecomment-1513184940)
I do think there's an unforeseen cost in storing these dust UTXOs, which might result in people willing to pay into a pot to have them cleared up - a bit like people grouping together to clean up the beaches of litter. If people didn't have to pay to pick-up their own dust, then it dust-picking might occur more often.
💬 0xB10C commented on pull request "kernel: chainparams updates for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27482#discussion_r1170066121)
2429000 on testnet might be too recent according to the docs: "Testnet should be set with a height some tens of thousands back from the tip, due to reorgs there.". It's currently 428 blocks old. There are still frequent [reorgs on testnet](https://fork.observer/?network=2), with the youngest being at 2428585. However, most reorgs on testnet I've seen are one-block reorgs and wouldn't affect this.
(https://github.com/bitcoin/bitcoin/pull/27482#discussion_r1170066121)
2429000 on testnet might be too recent according to the docs: "Testnet should be set with a height some tens of thousands back from the tip, due to reorgs there.". It's currently 428 blocks old. There are still frequent [reorgs on testnet](https://fork.observer/?network=2), with the youngest being at 2428585. However, most reorgs on testnet I've seen are one-block reorgs and wouldn't affect this.
💬 fanquake commented on pull request "kernel: chainparams updates for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27482#discussion_r1170070814)
It will also be much deeper once 25.x is actually released.
(https://github.com/bitcoin/bitcoin/pull/27482#discussion_r1170070814)
It will also be much deeper once 25.x is actually released.
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1170076066)
Because the other one is a return statement (the flow breaks there) while this one is a setter.
Plus, when the line is too long, like this one, I think that braces help readability.
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1170076066)
Because the other one is a return statement (the flow breaks there) while this one is a setter.
Plus, when the line is too long, like this one, I think that braces help readability.
👍 hebasto approved a pull request: "[24.x] Additional backports for 24.1"
(https://github.com/bitcoin/bitcoin/pull/27474#pullrequestreview-1390181803)
re-ACK dc711fbd32653b09e196f72942106114a32353f4
(https://github.com/bitcoin/bitcoin/pull/27474#pullrequestreview-1390181803)
re-ACK dc711fbd32653b09e196f72942106114a32353f4
💬 achow101 commented on pull request "doc: remove outdated version number usage from release-process":
(https://github.com/bitcoin/bitcoin/pull/27484#issuecomment-1513205003)
ACK fde224a6610699a6a28cc27e299ac14cbf7e16ca
(https://github.com/bitcoin/bitcoin/pull/27484#issuecomment-1513205003)
ACK fde224a6610699a6a28cc27e299ac14cbf7e16ca
🚀 achow101 merged a pull request: "doc: remove outdated version number usage from release-process"
(https://github.com/bitcoin/bitcoin/pull/27484)
(https://github.com/bitcoin/bitcoin/pull/27484)
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1170086188)
yes. `BLANK_BIRTH_TIME` is for blank wallets. Blank wallets are wallets with no keys (no spkm exist). So it's ok to skip blocks as the wallet isn't going to find anything on them.
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1170086188)
yes. `BLANK_BIRTH_TIME` is for blank wallets. Blank wallets are wallets with no keys (no spkm exist). So it's ok to skip blocks as the wallet isn't going to find anything on them.
🤔 jonatack reviewed a pull request: "[24.x] Additional backports for 24.1"
(https://github.com/bitcoin/bitcoin/pull/27474#pullrequestreview-1390197262)
ACK dc711fbd32653b09e196f72942106114a32353f4
Pull description is missing some of the changes.
(https://github.com/bitcoin/bitcoin/pull/27474#pullrequestreview-1390197262)
ACK dc711fbd32653b09e196f72942106114a32353f4
Pull description is missing some of the changes.
💬 0xB10C commented on pull request "kernel: chainparams updates for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27482#discussion_r1170091153)
Getting 502 GB in `blocks/` and 5.1GB in `chainstate/`. The new values seem fine as they should include enough overhead.
(https://github.com/bitcoin/bitcoin/pull/27482#discussion_r1170091153)
Getting 502 GB in `blocks/` and 5.1GB in `chainstate/`. The new values seem fine as they should include enough overhead.
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1170098672)
The id is the db lookup key for the descriptor, and also a way to detect db corruption.
We could clean it up from here, yeah. And probably, depending on its usage, we should also think about caching it.
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1170098672)
The id is the db lookup key for the descriptor, and also a way to detect db corruption.
We could clean it up from here, yeah. And probably, depending on its usage, we should also think about caching it.