✅ 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
💬 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_r1169161201)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
It seems like the code in this lambda would be simpler it avoided calling GetTxDepthInMainChain. The following should be equivalent:
```c++
// If the orig tx was not in block/mempool, none of its spends can be.
assert(!wtx.isConfirmed());
assert(!wtx.InMempool());
// If already conflicted or abandoned, no need to set abandoned.
if (!wtx.isConflicted() && !wtx.isAbandon
...
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169161201)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
It seems like the code in this lambda would be simpler it avoided calling GetTxDepthInMainChain. The following should be equivalent:
```c++
// If the orig tx was not in block/mempool, none of its spends can be.
assert(!wtx.isConfirmed());
assert(!wtx.InMempool());
// If already conflicted or abandoned, no need to set abandoned.
if (!wtx.isConflicted() && !wtx.isAbandon
...
💬 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_r1169145950)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
I think as long as this code is still calling GetTxDepthInMainChain, it would be better to keep the 3 comments that were here instead of deleting them, because they explain why the asserts are valid and why conflicted transactions are ignored
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169145950)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
I think as long as this code is still calling GetTxDepthInMainChain, it would be better to keep the 3 comments that were here instead of deleting them, because they explain why the asserts are valid and why conflicted transactions are ignored
💬 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_r1169166456)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This also seems like a useful comment. Would keep instead of deleting
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169166456)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This also seems like a useful comment. Would keep instead of deleting
💬 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_r1169172208)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This code pretty opaque, would suggest copying the previous comment here "Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too" or something equivalent.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169172208)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
This code pretty opaque, would suggest copying the previous comment here "Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too" or something equivalent.
💬 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_r1169173396)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
Would suggest keeping the previous comment here instead of deleting it. It's not obvious what the check means or what's happening in the block:
```c++
// Block is 'more conflicted' than current confirm; update.
// Mark transaction as conflicted with this block.
```
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1169173396)
In commit "wallet: introduce generic recursive tx state updating function" (f998cdbc689cc65ff0aca98ce63758c59786c1db)
Would suggest keeping the previous comment here instead of deleting it. It's not obvious what the check means or what's happening in the block:
```c++
// Block is 'more conflicted' than current confirm; update.
// Mark transaction as conflicted with this block.
```
💬 mzumsande commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169169601)
This non-deterministic test could fail intermittently, if `i2p_addr` collides with `addr1` (I tried it, and it happened to me after ~4000 runs).
It would be good to change the order, such that we first add an address to tried (via new), and only add another addr to new when the first has been moved over to tried.
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169169601)
This non-deterministic test could fail intermittently, if `i2p_addr` collides with `addr1` (I tried it, and it happened to me after ~4000 runs).
It would be good to change the order, such that we first add an address to tried (via new), and only add another addr to new when the first has been moved over to tried.
🤔 mzumsande reviewed a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214#pullrequestreview-1388792230)
Code Review ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6
I reviewed the PR again - although I'm a coauthor, so not sure about the etiquette of ack'ing.
I think there is a small bug in the non-deterministic test that could make it fail on rare occasions and should be fixed - here or in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/27214#pullrequestreview-1388792230)
Code Review ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6
I reviewed the PR again - although I'm a coauthor, so not sure about the etiquette of ack'ing.
I think there is a small bug in the non-deterministic test that could make it fail on rare occasions and should be fixed - here or in a follow-up.
💬 mzumsande commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169178603)
nit: mentioning the "empty pair" only in the first case makes it sound as if this wasn't one of the possible outcomes in the second case (but it is).
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169178603)
nit: mentioning the "empty pair" only in the first case makes it sound as if this wasn't one of the possible outcomes in the second case (but it is).
💬 theuni commented on pull request "depends: fix compiling bdb with clang-16 on aarch64":
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1511986632)
Ok, yeah, I guess my question was: why isn't this slipping into a "CC=clang" build. But point taken that's not really a supported combo.
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1511986632)
Ok, yeah, I guess my question was: why isn't this slipping into a "CC=clang" build. But point taken that's not really a supported combo.
💬 satsie commented on issue "Add per message stats to getnettotals rpc":
(https://github.com/bitcoin/bitcoin/issues/26337#issuecomment-1512090103)
Hi, I'm back again :laughing:
Here is the latest code I have: https://github.com/satsie/bitcoin/pull/5
It incorporates a bunch of feedback I got from the code I previously posted. The changes are mostly under the hood and have all been documented in the new PR's description for anyone that is curious. Design-wise, the only noteworthy thing is 'direction' is now available as a "aggregate type" (the term I am now using instead of "filter"). So now, results can be aggregated by:
- direction
...
(https://github.com/bitcoin/bitcoin/issues/26337#issuecomment-1512090103)
Hi, I'm back again :laughing:
Here is the latest code I have: https://github.com/satsie/bitcoin/pull/5
It incorporates a bunch of feedback I got from the code I previously posted. The changes are mostly under the hood and have all been documented in the new PR's description for anyone that is curious. Design-wise, the only noteworthy thing is 'direction' is now available as a "aggregate type" (the term I am now using instead of "filter"). So now, results can be aggregated by:
- direction
...