💬 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).
👍 stickies-v approved a pull request: "test: add regression tests for #27468 (invalid URI segfaults)"
(https://github.com/bitcoin/bitcoin/pull/27477#pullrequestreview-1388602489)
ACK 6a77d290da589bd5620585def5bfc019e242e189
The URI character validation/throwing logic is to be moved away from the query parameter logic (which would make it independent from these 3 endpoints), but this seems like a good addition for the follow-up to build on.
(https://github.com/bitcoin/bitcoin/pull/27477#pullrequestreview-1388602489)
ACK 6a77d290da589bd5620585def5bfc019e242e189
The URI character validation/throwing logic is to be moved away from the query parameter logic (which would make it independent from these 3 endpoints), but this seems like a good addition for the follow-up to build on.
🤔 glozow reviewed a pull request: "rpc: Add importmempool RPC"
(https://github.com/bitcoin/bitcoin/pull/27460#pullrequestreview-1388599658)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27460#pullrequestreview-1388599658)
Concept ACK
💬 glozow commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1169040970)
`PrioritiseTransaction` applies the delta on top, so if txmempool already has a tx prioritised + this mempool.dat contains priority for it, this will stack them instead of rewriting the existing priority. I don't know if this is problematic, but probably good to keep in mind? (Wrote a test)
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1169040970)
`PrioritiseTransaction` applies the delta on top, so if txmempool already has a tx prioritised + this mempool.dat contains priority for it, this will stack them instead of rewriting the existing priority. I don't know if this is problematic, but probably good to keep in mind? (Wrote a test)
💬 glozow commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1169040746)
Wrote a test, feel free to take / not take: https://github.com/glozow/bitcoin/commit/893f79c5ff612122e5b98d65b9859e7260c51a95#diff-99a8c43ebc6b8546e5fb9bf74032e96a6d3376b65655837e12454425098b9ad3R222
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1169040746)
Wrote a test, feel free to take / not take: https://github.com/glozow/bitcoin/commit/893f79c5ff612122e5b98d65b9859e7260c51a95#diff-99a8c43ebc6b8546e5fb9bf74032e96a6d3376b65655837e12454425098b9ad3R222
💬 glozow commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1169081824)
no special meaning, just me implementing "find the min" in a very elementary way :sweat_smile: will fix if I retouch
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1169081824)
no special meaning, just me implementing "find the min" in a very elementary way :sweat_smile: will fix if I retouch