💬 l0rinc commented on pull request "[IBD] precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2395190917)
You mean
```C++
return (*Assert(m_hasher))(wtxid.ToUint256()) & 0xffffffffffffL;
```
would be more explicit than
```C++
return m_hasher.value()(wtxid.ToUint256()) & 0xffffffffffffL;
```
?
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2395190917)
You mean
```C++
return (*Assert(m_hasher))(wtxid.ToUint256()) & 0xffffffffffffL;
```
would be more explicit than
```C++
return m_hasher.value()(wtxid.ToUint256()) & 0xffffffffffffL;
```
?
💬 tyuxar commented on issue "Higher **reported** memory usage of Bitcoin Core after version 29":
(https://github.com/bitcoin/bitcoin/issues/33351#issuecomment-3357203898)
It is not just a reporting issue, there is a problem with the memory use of v29.
I've been running Core in a FreeBSD jail for quite a long time (it's at 14.3p4 right now) and has been using the binary pkg.
v28 used at most approximately 1-1.5 GB, but usually it was hovering around 6-800 MB RSS.
v29 went up to slightly over 3GB shortly after startup, and remained there for 24+ hours. This is RSS! The virtual size was 12+ GB (I assume b/c of the mentioned LevelDB memory mapping).
As I'm runnin
...
(https://github.com/bitcoin/bitcoin/issues/33351#issuecomment-3357203898)
It is not just a reporting issue, there is a problem with the memory use of v29.
I've been running Core in a FreeBSD jail for quite a long time (it's at 14.3p4 right now) and has been using the binary pkg.
v28 used at most approximately 1-1.5 GB, but usually it was hovering around 6-800 MB RSS.
v29 went up to slightly over 3GB shortly after startup, and remained there for 24+ hours. This is RSS! The virtual size was 12+ GB (I assume b/c of the mentioned LevelDB memory mapping).
As I'm runnin
...
💬 cedwies commented on pull request "Avoid file overwriting in fallback `AllocateFileRange` implementation":
(https://github.com/bitcoin/bitcoin/pull/33164#discussion_r2395335416)
I would be a bit hesitant about mixing `lseek` with `FILE*`, since `stdio` has its own buffering. From what I read, a safer option might be to stick with the `stdio` layer (`fseeko`/`ftello`) or call `fstat` on the `fd` after flushing. Even though I expect mixing the two layers to work most of the time, it can be risky because stdio may have buffered data or be tracking the file position separately from the kernel. I think it's safer to either stick to _stdio_ **or** _fd-only.
(https://github.com/bitcoin/bitcoin/pull/33164#discussion_r2395335416)
I would be a bit hesitant about mixing `lseek` with `FILE*`, since `stdio` has its own buffering. From what I read, a safer option might be to stick with the `stdio` layer (`fseeko`/`ftello`) or call `fstat` on the `fd` after flushing. Even though I expect mixing the two layers to work most of the time, it can be risky because stdio may have buffered data or be tracking the file position separately from the kernel. I think it's safer to either stick to _stdio_ **or** _fd-only.
💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2395336697)
how so? Care to explain? :)
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2395336697)
how so? Care to explain? :)
💬 ryanofsky commented on pull request "init: Signal m_tip_block_cv on Ctrl-C":
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3357409495)
<!-- begin push-2 -->
Updated 4ee8e2248e54a33f0a3a2e808d46a7b0b8ea7bc3 -> 68cad90dace40f7a015ca4ff81b878fc8fdc1dd5 ([`pr/sigwait.1`](https://github.com/ryanofsky/bitcoin/commits/pr/sigwait.1) -> [`pr/sigwait.2`](https://github.com/ryanofsky/bitcoin/commits/pr/sigwait.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/sigwait.1..pr/sigwait.2))<!-- end --> to fix CI failures, also added more documentation and explanation in commit description
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3357409495)
<!-- begin push-2 -->
Updated 4ee8e2248e54a33f0a3a2e808d46a7b0b8ea7bc3 -> 68cad90dace40f7a015ca4ff81b878fc8fdc1dd5 ([`pr/sigwait.1`](https://github.com/ryanofsky/bitcoin/commits/pr/sigwait.1) -> [`pr/sigwait.2`](https://github.com/ryanofsky/bitcoin/commits/pr/sigwait.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/sigwait.1..pr/sigwait.2))<!-- end --> to fix CI failures, also added more documentation and explanation in commit description
🚀 achow101 merged a pull request: "[28.x] backports + 28.3rc1"
(https://github.com/bitcoin/bitcoin/pull/33476)
(https://github.com/bitcoin/bitcoin/pull/33476)
👍 hodlinator approved a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326#pullrequestreview-3290340107)
re-ACK 87e7f37918d42c28033e9f684db52f94eeed617b
Changes since previous ACK:
* Avoided function overloading and documented that hostnames can be used in more places, not just raw IP addresses and the like.
* `--auto`
(https://github.com/bitcoin/bitcoin/pull/32326#pullrequestreview-3290340107)
re-ACK 87e7f37918d42c28033e9f684db52f94eeed617b
Changes since previous ACK:
* Avoided function overloading and documented that hostnames can be used in more places, not just raw IP addresses and the like.
* `--auto`
👍 theStack approved a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3290195368)
Code-review ACK ac599c4a9cb3b2d424932d3fd91f9eed17426827 :old_key:
Happy to review potential follow-ups.
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3290195368)
Code-review ACK ac599c4a9cb3b2d424932d3fd91f9eed17426827 :old_key:
Happy to review potential follow-ups.
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2395372134)
nit: seems that this shouldn't be in upper-case, if it's not really a constant
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2395372134)
nit: seems that this shouldn't be in upper-case, if it's not really a constant
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2395283294)
nit: could move the MUSIG_CHAINCODE constant from the header to musig.cpp, as its only used there now
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2395283294)
nit: could move the MUSIG_CHAINCODE constant from the header to musig.cpp, as its only used there now
📝 sipa opened a pull request: "Improve LastCommonAncestor performance + add tests"
(https://github.com/bitcoin/bitcoin/pull/33515)
In theory, the `LastCommonAncestor` function in chain.cpp can take $\mathcal{O}(n)$ time, walking over the entire chain, if the forking point is very early, which could take ~milliseconds. I expect this to be very rare in normal occurrences, but it seems nontrivial to reason about worst cases as it's accessible from several places in net_processing.
This PR modifies the algorithm to make use of the `CBlockIndex::pskip` skip pointers to find the forking point in sublinear time (a simulation sh
...
(https://github.com/bitcoin/bitcoin/pull/33515)
In theory, the `LastCommonAncestor` function in chain.cpp can take $\mathcal{O}(n)$ time, walking over the entire chain, if the forking point is very early, which could take ~milliseconds. I expect this to be very rare in normal occurrences, but it seems nontrivial to reason about worst cases as it's accessible from several places in net_processing.
This PR modifies the algorithm to make use of the `CBlockIndex::pskip` skip pointers to find the forking point in sublinear time (a simulation sh
...
💬 ryanofsky commented on issue "How to backport libmultiprocess changes?":
(https://github.com/bitcoin/bitcoin/issues/33439#issuecomment-3357606911)
> when the subtree bump is created on master, it could make sense to base it before the branch-off point, so that the exact same commit ID can be merged into both branches
This is a nice idea. Right now it looks like https://github.com/bitcoin/bitcoin/pull/33322 was the last update merged before the [30.x](https://github.com/bitcoin/bitcoin/commits/30.x) branch point and https://github.com/bitcoin/bitcoin/pull/33412 was more recently merged after, so it may be too late to use that strategy in t
...
(https://github.com/bitcoin/bitcoin/issues/33439#issuecomment-3357606911)
> when the subtree bump is created on master, it could make sense to base it before the branch-off point, so that the exact same commit ID can be merged into both branches
This is a nice idea. Right now it looks like https://github.com/bitcoin/bitcoin/pull/33322 was the last update merged before the [30.x](https://github.com/bitcoin/bitcoin/commits/30.x) branch point and https://github.com/bitcoin/bitcoin/pull/33412 was more recently merged after, so it may be too late to use that strategy in t
...
💬 davidgumberg commented on pull request "depends: static libxcb-cursor":
(https://github.com/bitcoin/bitcoin/pull/33434#issuecomment-3357685190)
Addendum ACK https://github.com/bitcoin/bitcoin/commit/eca50854e1cb04e20478bd3df4762e18520a3611
After uninstalling and reinstalling guix on the machine that encountered the build issue earlier, this branch built fine without any errors.
```console
$ ./contrib/guix/guix-build
$ uname -m && find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
```
```
x86_64
cf7f86d91288477c11afdc767969d6b19b0ba1e62c73924e72b22a252737465
...
(https://github.com/bitcoin/bitcoin/pull/33434#issuecomment-3357685190)
Addendum ACK https://github.com/bitcoin/bitcoin/commit/eca50854e1cb04e20478bd3df4762e18520a3611
After uninstalling and reinstalling guix on the machine that encountered the build issue earlier, this branch built fine without any errors.
```console
$ ./contrib/guix/guix-build
$ uname -m && find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
```
```
x86_64
cf7f86d91288477c11afdc767969d6b19b0ba1e62c73924e72b22a252737465
...
💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2395570236)
This is out of scope, but I can address this in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2395570236)
This is out of scope, but I can address this in a follow-up.
💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2395573945)
This PR does not change the error handling here, but I can address this in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2395573945)
This PR does not change the error handling here, but I can address this in a follow-up.
💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#issuecomment-3357753607)
Rebased to address a small merge conflict, since some instances of `LoadWallet()` (renamed PopulateWalletFromDB in this branch) were removed in #33082.
(https://github.com/bitcoin/bitcoin/pull/32636#issuecomment-3357753607)
Rebased to address a small merge conflict, since some instances of `LoadWallet()` (renamed PopulateWalletFromDB in this branch) were removed in #33082.
🤔 furszy reviewed a pull request: "Improve LastCommonAncestor performance + add tests"
(https://github.com/bitcoin/bitcoin/pull/33515#pullrequestreview-3290718016)
Code reviewed ACK [4fe3de2](https://github.com/bitcoin/bitcoin/pull/33515/commits/4fe3de2aa7fda58d5c56987aee13ae87191da1c6)
(https://github.com/bitcoin/bitcoin/pull/33515#pullrequestreview-3290718016)
Code reviewed ACK [4fe3de2](https://github.com/bitcoin/bitcoin/pull/33515/commits/4fe3de2aa7fda58d5c56987aee13ae87191da1c6)
💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2395761545)
It seems https://github.com/bitcoin/bitcoin/pull/32602/files#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R312-R321 revealed that there's a silent conflict here, we need the [first commit](https://github.com/bitcoin/bitcoin/pull/32313/commits/85979e2015344f05aa60db52b4a76eeadef216ad) from https://github.com/bitcoin/bitcoin/pull/32313 for this to pass after rebase
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2395761545)
It seems https://github.com/bitcoin/bitcoin/pull/32602/files#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R312-R321 revealed that there's a silent conflict here, we need the [first commit](https://github.com/bitcoin/bitcoin/pull/32313/commits/85979e2015344f05aa60db52b4a76eeadef216ad) from https://github.com/bitcoin/bitcoin/pull/32313 for this to pass after rebase
💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3357991920)
Rebased and adjusted the commits to minimize conflict with https://github.com/bitcoin/bitcoin/pull/33512
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3357991920)
Rebased and adjusted the commits to minimize conflict with https://github.com/bitcoin/bitcoin/pull/33512
👍 TheCharlatan approved a pull request: "init: Fix Ctrl-C shutdown hangs during wait calls"
(https://github.com/bitcoin/bitcoin/pull/33511#pullrequestreview-3291091712)
ACK ef7be125ea92e182f087d2c8e3d5c605b8df31d9
This should be backported in my opinion.
(https://github.com/bitcoin/bitcoin/pull/33511#pullrequestreview-3291091712)
ACK ef7be125ea92e182f087d2c8e3d5c605b8df31d9
This should be backported in my opinion.