π¬ Crypt-iQ commented on issue "ASAN use-after-free in `m_reconnections`":
(https://github.com/bitcoin/bitcoin/issues/33615#issuecomment-3512126349)
@big14way I was planning on fixing this issue myself, but if you want to fix it, feel free. I think it'd be valuable if you were able to reproduce the use-after-free to make sure that the issue is fixed.
(https://github.com/bitcoin/bitcoin/issues/33615#issuecomment-3512126349)
@big14way I was planning on fixing this issue myself, but if you want to fix it, feel free. I think it'd be valuable if you were able to reproduce the use-after-free to make sure that the issue is fixed.
π¬ optout21 commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3512132153)
Concept ACK
I can't judge the probability of mutability check here needed again in the future, but in that case, it can be added.
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3512132153)
Concept ACK
I can't judge the probability of mutability check here needed again in the future, but in that case, it can be added.
π fanquake merged a pull request: "test: rpc_bind: Skip nonloopback test if no such address is found"
(https://github.com/bitcoin/bitcoin/pull/33433)
(https://github.com/bitcoin/bitcoin/pull/33433)
β
fanquake closed a pull request: "Add option dbfilesize to control LevelDB target ("max") file size"
(https://github.com/bitcoin/bitcoin/pull/30059)
(https://github.com/bitcoin/bitcoin/pull/30059)
π¬ fanquake commented on pull request "Add option dbfilesize to control LevelDB target ("max") file size":
(https://github.com/bitcoin/bitcoin/pull/30059#issuecomment-3512199526)
Closing for now.
(https://github.com/bitcoin/bitcoin/pull/30059#issuecomment-3512199526)
Closing for now.
π¬ sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2510907674)
@ajtowns I don't think a virtual move assignment would actually help. It's true that there are risks when performing (move) assignments across types, but it doesn't seem like making it virtual affects those much.
With the code, as it exists in this PR, the risk is something like:
```
TxGraph::Ref ref = ...;
TxMempoolEntry entry = ...;
...
ref = std::move(entry);
// or
entry = std::move(ref);
```
The first of which will "excise" the ref from the entry, leaving the entry ref-less. Th
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2510907674)
@ajtowns I don't think a virtual move assignment would actually help. It's true that there are risks when performing (move) assignments across types, but it doesn't seem like making it virtual affects those much.
With the code, as it exists in this PR, the risk is something like:
```
TxGraph::Ref ref = ...;
TxMempoolEntry entry = ...;
...
ref = std::move(entry);
// or
entry = std::move(ref);
```
The first of which will "excise" the ref from the entry, leaving the entry ref-less. Th
...
π maflcko opened a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842)
All supported operating systems that previously came with at least g++-11, also come with at least g++-12, so bumping the minimum should be fine.
(https://github.com/bitcoin/bitcoin/pull/33842)
All supported operating systems that previously came with at least g++-11, also come with at least g++-12, so bumping the minimum should be fine.
π¬ glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2510927772)
Looks good to me
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2510927772)
Looks good to me
π vasild opened a pull request: "test: add tests for private broadcast"
(https://github.com/bitcoin/bitcoin/pull/33843)
This PR contains https://github.com/bitcoin/bitcoin/pull/29415 + two more commits that add functional and fuzz tests.
Putting those in a separate PR not to burden the main one, since the tests are extensive:
code: `1036 insertions(+), 91 deletions(-)`
tests: `548 insertions(+), 5 deletions(-)`
(https://github.com/bitcoin/bitcoin/pull/33843)
This PR contains https://github.com/bitcoin/bitcoin/pull/29415 + two more commits that add functional and fuzz tests.
Putting those in a separate PR not to burden the main one, since the tests are extensive:
code: `1036 insertions(+), 91 deletions(-)`
tests: `548 insertions(+), 5 deletions(-)`
π¬ vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3512327977)
`29ef3c62de...d419fbc42b`: drop the last commit which contained the functional tests. That functional test + a fuzz test now live in a separate PR: https://github.com/bitcoin/bitcoin/pull/33843. This reduces the main PR from
`1451 insertions(+), 91 deletions(-)` to
`1036 insertions(+), 91 deletions(-)`.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3512327977)
`29ef3c62de...d419fbc42b`: drop the last commit which contained the functional tests. That functional test + a fuzz test now live in a separate PR: https://github.com/bitcoin/bitcoin/pull/33843. This reduces the main PR from
`1451 insertions(+), 91 deletions(-)` to
`1036 insertions(+), 91 deletions(-)`.
π¬ dergoegge commented on pull request "test: add tests for private broadcast":
(https://github.com/bitcoin/bitcoin/pull/33843#issuecomment-3512335697)
Can you elaborate on the rational here? Why would the tests burden the main change? In my opinion, the purpose of tests should be to make it easier to review a code change and gain confidence in it's correctness etc. Splitting them from the code change seems counter productive.
(https://github.com/bitcoin/bitcoin/pull/33843#issuecomment-3512335697)
Can you elaborate on the rational here? Why would the tests burden the main change? In my opinion, the purpose of tests should be to make it easier to review a code change and gain confidence in it's correctness etc. Splitting them from the code change seems counter productive.
π¬ hebasto commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3512348200)
> > Even if itβs been fixed upstream,
>
> It's not clear to me if it has been fixed or not, can you link to the relevant change / issue?
See https://bugreports.qt.io/browse/QTBUG-141858.
> I can cherry-pick the change above in, if you create a commit, and add a comment inline, for when the change can be dropped.
Feel free to pick top two commits from https://github.com/hebasto/bitcoin/commits/pr32482/1110/.
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3512348200)
> > Even if itβs been fixed upstream,
>
> It's not clear to me if it has been fixed or not, can you link to the relevant change / issue?
See https://bugreports.qt.io/browse/QTBUG-141858.
> I can cherry-pick the change above in, if you create a commit, and add a comment inline, for when the change can be dropped.
Feel free to pick top two commits from https://github.com/hebasto/bitcoin/commits/pr32482/1110/.
π¬ fanquake commented on pull request "test: add tests for private broadcast":
(https://github.com/bitcoin/bitcoin/pull/33843#issuecomment-3512354999)
Concept NACK to deferring adding tests until after adding a new feature.
(https://github.com/bitcoin/bitcoin/pull/33843#issuecomment-3512354999)
Concept NACK to deferring adding tests until after adding a new feature.
π€ hebasto reviewed a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3443882743)
Such a change might also require updating the build guides. For instance, the following commands on Ubuntu 22.04 would use GCC 11:
```
sudo apt-get install build-essential ...
cmake --build build
```
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3443882743)
Such a change might also require updating the build guides. For instance, the following commands on Ubuntu 22.04 would use GCC 11:
```
sudo apt-get install build-essential ...
cmake --build build
```
β
vasild closed a pull request: "test: add tests for private broadcast"
(https://github.com/bitcoin/bitcoin/pull/33843)
(https://github.com/bitcoin/bitcoin/pull/33843)
π¬ vasild commented on pull request "test: add tests for private broadcast":
(https://github.com/bitcoin/bitcoin/pull/33843#issuecomment-3512512550)
Closing after discussion at https://www.erisian.com.au/bitcoin-core-dev/log-2025-11-10.html#l-236
(https://github.com/bitcoin/bitcoin/pull/33843#issuecomment-3512512550)
Closing after discussion at https://www.erisian.com.au/bitcoin-core-dev/log-2025-11-10.html#l-236
π¬ vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3512531635)
`d419fbc42b..29ef3c62de`: restore back the test here and close https://github.com/bitcoin/bitcoin/pull/33843 after discussion at https://www.erisian.com.au/bitcoin-core-dev/log-2025-11-10.html#l-236
If people review up to but not including the test, they can ACK the last but one commit.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3512531635)
`d419fbc42b..29ef3c62de`: restore back the test here and close https://github.com/bitcoin/bitcoin/pull/33843 after discussion at https://www.erisian.com.au/bitcoin-core-dev/log-2025-11-10.html#l-236
If people review up to but not including the test, they can ACK the last but one commit.
π¬ fanquake commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3512546980)
> See https://bugreports.qt.io/browse/QTBUG-141858.
> Feel free to pick
Thanks. I've pulled those two in here.
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3512546980)
> See https://bugreports.qt.io/browse/QTBUG-141858.
> Feel free to pick
Thanks. I've pulled those two in here.
π¬ achow101 commented on pull request "wallet: Always rewrite tx records during migration":
(https://github.com/bitcoin/bitcoin/pull/32985#issuecomment-3512556430)
> these changes are persisted during `ReorderTransactions()`
Not for migration since legacy wallets are only opened with a `BerkeleyRODatabase` which does not write anything.
(https://github.com/bitcoin/bitcoin/pull/32985#issuecomment-3512556430)
> these changes are persisted during `ReorderTransactions()`
Not for migration since legacy wallets are only opened with a `BerkeleyRODatabase` which does not write anything.
π¬ maflcko commented on pull request "build: Bump g++ minimum supported version to 12":
(https://github.com/bitcoin/bitcoin/pull/33842#issuecomment-3512575639)
> Such a change might also require updating the build guides. For instance, the following commands on Ubuntu 22.04 would use GCC 11:
I am happy to add a general note that the latest stable or LTS is recommended and that earlier releases may need the user to install the required minimum dependencies.
(https://github.com/bitcoin/bitcoin/pull/33842#issuecomment-3512575639)
> Such a change might also require updating the build guides. For instance, the following commands on Ubuntu 22.04 would use GCC 11:
I am happy to add a general note that the latest stable or LTS is recommended and that earlier releases may need the user to install the required minimum dependencies.