💬 MarcoFalke commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168831940)
Any reason to use `int64_t`, when the block time is denoted in a type-safe `NodeSeconds`?
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168831940)
Any reason to use `int64_t`, when the block time is denoted in a type-safe `NodeSeconds`?
📝 fanquake opened a pull request: "[23.x] Additional backports for 23.x"
(https://github.com/bitcoin/bitcoin/pull/27475)
Any further backports for 23.x. Currently just:
* 1bdbbbdc46c4e50bf07bc362e7e391ea1a53ea2f from https://github.com/bitcoin/bitcoin/pull/25436 - which fixes building QT in depends with GCC > 12.1
We could also add #27462, to fix building BDB with newer Clangs on aarch64.
(https://github.com/bitcoin/bitcoin/pull/27475)
Any further backports for 23.x. Currently just:
* 1bdbbbdc46c4e50bf07bc362e7e391ea1a53ea2f from https://github.com/bitcoin/bitcoin/pull/25436 - which fixes building QT in depends with GCC > 12.1
We could also add #27462, to fix building BDB with newer Clangs on aarch64.
🚀 fanquake merged a pull request: "depends: fix compiling bdb with clang-16 on aarch64"
(https://github.com/bitcoin/bitcoin/pull/27462)
(https://github.com/bitcoin/bitcoin/pull/27462)
🚀 fanquake merged a pull request: "test: fix bumpfee 'spend_one_input' occasional failure"
(https://github.com/bitcoin/bitcoin/pull/27471)
(https://github.com/bitcoin/bitcoin/pull/27471)
💬 theuni commented on pull request "depends: fix compiling bdb with clang-16 on aarch64":
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1511612965)
Wait, did we ever figure out why this only happens when not cross-compiling? That's fishy to me.
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1511612965)
Wait, did we ever figure out why this only happens when not cross-compiling? That's fishy to me.
💬 hebasto commented on pull request "Initialise DBus notifications in another thread":
(https://github.com/bitcoin-core/gui/pull/152#issuecomment-1511682423)
Closing this due to lack of activity. Feel free to reopen.
(https://github.com/bitcoin-core/gui/pull/152#issuecomment-1511682423)
Closing this due to lack of activity. Feel free to reopen.
✅ hebasto closed a pull request: "Initialise DBus notifications in another thread"
(https://github.com/bitcoin-core/gui/pull/152)
(https://github.com/bitcoin-core/gui/pull/152)
💬 ryanofsky commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1168973760)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
> I think that this is a good idea, but would out of the scope of this PR and would warrant more discussion, since then it would be notifying for tx state updates that it previously did not.
Agree we should avoid changing behavior, but I think the current commit might also be changing behavior by notifying before calling MarkDirty and WriteTx instead of after.
I think th
...
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1168973760)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
> I think that this is a good idea, but would out of the scope of this PR and would warrant more discussion, since then it would be notifying for tx state updates that it previously did not.
Agree we should avoid changing behavior, but I think the current commit might also be changing behavior by notifying before calling MarkDirty and WriteTx instead of after.
I think th
...
🤔 pinheadmz reviewed a pull request: "wallet: improve IBD sync time by skipping block scanning prior birth time"
(https://github.com/bitcoin/bitcoin/pull/27469#pullrequestreview-1388370915)
Did some general code review. I see we already have `nCreateTime` for descriptors and part of this PR seems to be just bringing that up to the wallet level, so that's not a redundancy.
Also I don't know if this is a problematic edge case or it could just be a good way to test but you could create a key and send BTC to it in a block, then a few blocks later import into a new descriptor and rescan. The wallet *should* miss the first transaction.
(https://github.com/bitcoin/bitcoin/pull/27469#pullrequestreview-1388370915)
Did some general code review. I see we already have `nCreateTime` for descriptors and part of this PR seems to be just bringing that up to the wallet level, so that's not a redundancy.
Also I don't know if this is a problematic edge case or it could just be a good way to test but you could create a key and send BTC to it in a block, then a few blocks later import into a new descriptor and rescan. The wallet *should* miss the first transaction.
💬 pinheadmz commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168887852)
why not a single-line `if` like you do below ?
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168887852)
why not a single-line `if` like you do below ?
💬 pinheadmz commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168919382)
`birthtime == BLANK_BIRTH_TIME ` -- doesn't this mean if a wallet has no known birthtime, it will never process any blocks?
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168919382)
`birthtime == BLANK_BIRTH_TIME ` -- doesn't this mean if a wallet has no known birthtime, it will never process any blocks?
💬 pinheadmz commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168946262)
`id` is now an unused function argument `LoadDescriptorScriptPubKeyMan()`. Could clean that up? Actually I'm kinda curious why we ever needed to pass `id` if the value is always derivable from the descriptor itself.
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168946262)
`id` is now an unused function argument `LoadDescriptorScriptPubKeyMan()`. Could clean that up? Actually I'm kinda curious why we ever needed to pass `id` if the value is always derivable from the descriptor itself.
💬 pinheadmz commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168949017)
I think adding this method is a good idea, and cleans up all the `m_spk_managers[id] = ...` which I like. I wonder if that could just be a separate commit since its just a nice refactor? And then add `FIrstKeyTimeChanged()` next
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1168949017)
I think adding this method is a good idea, and cleans up all the `m_spk_managers[id] = ...` which I like. I wonder if that could just be a separate commit since its just a nice refactor? And then add `FIrstKeyTimeChanged()` next
💬 hebasto commented on pull request "Enable users to configure their monospace font specifically":
(https://github.com/bitcoin-core/gui/pull/497#issuecomment-1511686277)
Closing this due to lack of activity. Feel free to reopen.
(https://github.com/bitcoin-core/gui/pull/497#issuecomment-1511686277)
Closing this due to lack of activity. Feel free to reopen.
✅ hebasto closed a pull request: "Enable users to configure their monospace font specifically"
(https://github.com/bitcoin-core/gui/pull/497)
(https://github.com/bitcoin-core/gui/pull/497)
💬 fanquake commented on pull request "depends: fix compiling bdb with clang-16 on aarch64":
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1511691401)
This does happen when cross-compiling with Clang-16, i.e `clang-16 test.c --target=aarch64-linux-gnu`, where test.c contains the equivalent of the warning-causing code being dumped into conftest.c. However depends isn't really usable to easily do cross-compilation with Clang, except for where we've got it setup for macOS.
The fact that this doesn't fail in the same way, when using a different cross-compiler (GCC), cross-compiling on x86_64, seems like a potentially related, but separate issue
...
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1511691401)
This does happen when cross-compiling with Clang-16, i.e `clang-16 test.c --target=aarch64-linux-gnu`, where test.c contains the equivalent of the warning-causing code being dumped into conftest.c. However depends isn't really usable to easily do cross-compilation with Clang, except for where we've got it setup for macOS.
The fact that this doesn't fail in the same way, when using a different cross-compiler (GCC), cross-compiling on x86_64, seems like a potentially related, but separate issue
...
💬 hebasto commented on pull request "Point out position of invalid characters in Bech32 addresses":
(https://github.com/bitcoin-core/gui/pull/537#issuecomment-1511703226)
Closing this due to lack of activity. Feel free to reopen.
(https://github.com/bitcoin-core/gui/pull/537#issuecomment-1511703226)
Closing this due to lack of activity. Feel free to reopen.
✅ hebasto closed a pull request: "Point out position of invalid characters in Bech32 addresses"
(https://github.com/bitcoin-core/gui/pull/537)
(https://github.com/bitcoin-core/gui/pull/537)
📝 glozow opened a pull request: "mempool: keep CPFP'd transactions when loading from mempool.dat"
(https://github.com/bitcoin/bitcoin/pull/27476)
This contains functionality and is all the commits shared between #26711 and #25038 (distinct projects, see #27463).
This is built on top of #26933 - review that first.
**Problem**
When loading mempool.dat, we apply -minrelaytxfee and mempool min feerate on each transaction, meaning we'll reject transactions that may be CPFP'd by later transactions mempool.dat. Even without package relay, we can run into this problem if we are shrinking -maxmempool or raising -minrelaytxfee on a restart.
...
(https://github.com/bitcoin/bitcoin/pull/27476)
This contains functionality and is all the commits shared between #26711 and #25038 (distinct projects, see #27463).
This is built on top of #26933 - review that first.
**Problem**
When loading mempool.dat, we apply -minrelaytxfee and mempool min feerate on each transaction, meaning we'll reject transactions that may be CPFP'd by later transactions mempool.dat. Even without package relay, we can run into this problem if we are shrinking -maxmempool or raising -minrelaytxfee on a restart.
...
📝 theStack opened a pull request: "test: add regression tests for #27468 (invalid URI segfaults)"
(https://github.com/bitcoin/bitcoin/pull/27477)
Prior to PR #27468 (commit 11422cc5720c8d73a87600de8fe8abb156db80dc) all call-sites of `GetQueryParameter(...)` in the REST module could trigger a crash. Add missing test cases for all possible code-paths as a regression test, as a foundation for possible follow-up fixes (which aim to resolve this issue in a more general and robust way).
(https://github.com/bitcoin/bitcoin/pull/27477)
Prior to PR #27468 (commit 11422cc5720c8d73a87600de8fe8abb156db80dc) all call-sites of `GetQueryParameter(...)` in the REST module could trigger a crash. Add missing test cases for all possible code-paths as a regression test, as a foundation for possible follow-up fixes (which aim to resolve this issue in a more general and robust way).