👍 laanwj approved a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3512087837)
re-ACK 1b810390fe51b79c7063966c9722c01cb8a4f7bf
Since last review:
```
git range-diff 9b1a7c3e8dd78d97fbf47c2d056d043b05969176..89bbbbd257063118e6968c409e52632835b76ce8 17072f70051dc086ef57880cd14e102ed346c350..1b810390fe51b79c7063966c9722c01cb8a4f7bf
```
1. `test: Rename k1/k2 to k0/k1 in SipHash consistency tests:` - Reordering in testcase, no functional change
2. `refactor: Extract SipHash C0-C3 constants to class scope` - No changes
3. `optimization: Introduce PresaltedSipHasher for repe
...
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3512087837)
re-ACK 1b810390fe51b79c7063966c9722c01cb8a4f7bf
Since last review:
```
git range-diff 9b1a7c3e8dd78d97fbf47c2d056d043b05969176..89bbbbd257063118e6968c409e52632835b76ce8 17072f70051dc086ef57880cd14e102ed346c350..1b810390fe51b79c7063966c9722c01cb8a4f7bf
```
1. `test: Rename k1/k2 to k0/k1 in SipHash consistency tests:` - Reordering in testcase, no functional change
2. `refactor: Extract SipHash C0-C3 constants to class scope` - No changes
3. `optimization: Introduce PresaltedSipHasher for repe
...
💬 laanwj commented on pull request "guix: reduce allowed exported symbols":
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3582538390)
Nice. Concept ACK. Will do riscv64 guix build.
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3582538390)
Nice. Concept ACK. Will do riscv64 guix build.
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2565992040)
yeah, sry, I had found it already. I realized that it essentially just concerns text being prepended, so it is mostly whoever "tells the story" on this API. I will make the changes, or have made; need to (re)check.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2565992040)
yeah, sry, I had found it already. I realized that it essentially just concerns text being prepended, so it is mostly whoever "tells the story" on this API. I will make the changes, or have made; need to (re)check.
💬 gmaxwell commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2566047719)
Might be useful to specifically include something that describes it in the literature terms, like:
"Producing an optimal linearization of a cluster is an instance of the parametric maximal weight closure problem as found in the field of mineral extraction for open pit mining."
-- just to give future researchers a hook to the literature that contributors here initially lacked.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2566047719)
Might be useful to specifically include something that describes it in the literature terms, like:
"Producing an optimal linearization of a cluster is an instance of the parametric maximal weight closure problem as found in the field of mineral extraction for open pit mining."
-- just to give future researchers a hook to the literature that contributors here initially lacked.
💬 kevkevinpal commented on pull request "policy: Remove individual transaction <minrelay restriction":
(https://github.com/bitcoin/bitcoin/pull/33892#discussion_r2566086022)
why not drop the whole test as per this comment?
(https://github.com/bitcoin/bitcoin/pull/33892#discussion_r2566086022)
why not drop the whole test as per this comment?
💬 plebhash commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3582775203)
we merged https://github.com/stratum-mining/sv2-apps/pull/59 so deploying SRI Pool and/or JDC for testing can be done from `main` branch
> The node log messages should contain a destroy entry every time the client releases a template. It's probably also a good idea to have the client emit a log message when it discards a template.
I just did some tests with:
- Bitcoin Core v30 binary downloaded from official website
- `mainnet` (for frequent template updates)
- SRI Pool from commit https://git
...
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3582775203)
we merged https://github.com/stratum-mining/sv2-apps/pull/59 so deploying SRI Pool and/or JDC for testing can be done from `main` branch
> The node log messages should contain a destroy entry every time the client releases a template. It's probably also a good idea to have the client emit a log message when it discards a template.
I just did some tests with:
- Bitcoin Core v30 binary downloaded from official website
- `mainnet` (for frequent template updates)
- SRI Pool from commit https://git
...
📝 mzumsande opened a pull request: "test: add functional test for outbound connection management"
(https://github.com/bitcoin/bitcoin/pull/33954)
For #29415, vasild added the possibility to make outbound connections made "naturally" by the node from addrman entries, by redirecting them via a SOCKS5 proxy to python p2ps or other full nodes, so that the node under test behaves as if it is connected to normal non-local peers from arbitrary networks, while we have full control over the peers.
I think this feature is really cool and want to suggest to integrate it more prominently into the test framework ("`auto_outbound_mode`") in the firs
...
(https://github.com/bitcoin/bitcoin/pull/33954)
For #29415, vasild added the possibility to make outbound connections made "naturally" by the node from addrman entries, by redirecting them via a SOCKS5 proxy to python p2ps or other full nodes, so that the node under test behaves as if it is connected to normal non-local peers from arbitrary networks, while we have full control over the peers.
I think this feature is really cool and want to suggest to integrate it more prominently into the test framework ("`auto_outbound_mode`") in the firs
...
✅ sashass1315 closed a pull request: "coinstats: avoid unnecessary Coin copy in ApplyHash"
(https://github.com/bitcoin/bitcoin/pull/33410)
(https://github.com/bitcoin/bitcoin/pull/33410)
📝 sashass1315 opened a pull request: "leveldb: correct trailer boundary check in Reader::SkipToInitialBlock"
(https://github.com/bitcoin/bitcoin/pull/33955)
replaces the magic number 6 in the trailer boundary condition with kHeaderSize and corrects an off-by-one by expressing the rule as offset_in_block > kBlockSize - kHeaderSize, which is equivalent to offset_in_block >= kBlockSize - 6. According to
src/leveldb/doc/log_format.md:
“A record never starts within the last six bytes of a block”
and when exactly seven bytes remain a record may start (e.g., a zero-length FIRST), so the equal case must be treated as trailer. The writer already uses kH
...
(https://github.com/bitcoin/bitcoin/pull/33955)
replaces the magic number 6 in the trailer boundary condition with kHeaderSize and corrects an off-by-one by expressing the rule as offset_in_block > kBlockSize - kHeaderSize, which is equivalent to offset_in_block >= kBlockSize - 6. According to
src/leveldb/doc/log_format.md:
“A record never starts within the last six bytes of a block”
and when exactly seven bytes remain a record may start (e.g., a zero-length FIRST), so the equal case must be treated as trailer. The writer already uses kH
...
✅ fanquake closed a pull request: "contrib: turn off compression of macOS SDK to fix determinism (across distros)"
(https://github.com/bitcoin/bitcoin/pull/32009)
(https://github.com/bitcoin/bitcoin/pull/32009)
📝 fanquake reopened a pull request: "contrib: turn off compression of macOS SDK to fix determinism (across distros)"
(https://github.com/bitcoin/bitcoin/pull/32009)
This includes three changes. The first is to more selectively pick files for inclusion into our macOS SDK tarball (skip manpages, binaries etc), which is nice because it redues the size of the tarball (from ~80mb to 20mb), and makes the size increase that happens with the next commit, less-bad.
The second change removes compression of the tarball. Starting with Python 3.11, Pythons gzip might delegate to zlib. Depending on the OS, i.e Ubuntu vs Fedora, the underlying zlib implementation might
...
(https://github.com/bitcoin/bitcoin/pull/32009)
This includes three changes. The first is to more selectively pick files for inclusion into our macOS SDK tarball (skip manpages, binaries etc), which is nice because it redues the size of the tarball (from ~80mb to 20mb), and makes the size increase that happens with the next commit, less-bad.
The second change removes compression of the tarball. Starting with Python 3.11, Pythons gzip might delegate to zlib. Depending on the OS, i.e Ubuntu vs Fedora, the underlying zlib implementation might
...
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#issuecomment-3582846817)
@maflcko I expected that someone would squash upon integrating the work. Now squashed.
I included some of the changes from #33947, though not verbatim. It may be worth checking the changes again, as a whole.
(https://github.com/bitcoin/bitcoin/pull/33765#issuecomment-3582846817)
@maflcko I expected that someone would squash upon integrating the work. Now squashed.
I included some of the changes from #33947, though not verbatim. It may be worth checking the changes again, as a whole.
💬 fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3582852010)
New tarball is now available. Open/close to kick the CI.
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3582852010)
New tarball is now available. Open/close to kick the CI.
👋 mzumsande's pull request is ready for review: "test: add functional test for outbound connection management"
(https://github.com/bitcoin/bitcoin/pull/33954)
(https://github.com/bitcoin/bitcoin/pull/33954)
💬 janb84 commented on pull request "guix: reduce allowed exported symbols":
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3583157897)
my Guix Build Output (equal to fanquake)
**Host architecture:** `aarch64`
**Commit:** `2984690b20f7`
```shell
5e1eb207d68450278951fe45b8c06465beeb49af6a84ef12d037430dcd6519ad guix-build-2984690b20f7/output/aarch64-linux-gnu/SHA256SUMS.part
dd93dd1b964f0629eae5db5106ad1afc307f4317fdecd5f44223aa1942f56348 guix-build-2984690b20f7/output/aarch64-linux-gnu/bitcoin-2984690b20f7-aarch64-linux-gnu-debug.tar.gz
af0c96a7f3f1eb528e21af366d4fd6f585dd5b6620d147822f028bb4c14023f0 guix-build-29
...
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3583157897)
my Guix Build Output (equal to fanquake)
**Host architecture:** `aarch64`
**Commit:** `2984690b20f7`
```shell
5e1eb207d68450278951fe45b8c06465beeb49af6a84ef12d037430dcd6519ad guix-build-2984690b20f7/output/aarch64-linux-gnu/SHA256SUMS.part
dd93dd1b964f0629eae5db5106ad1afc307f4317fdecd5f44223aa1942f56348 guix-build-2984690b20f7/output/aarch64-linux-gnu/bitcoin-2984690b20f7-aarch64-linux-gnu-debug.tar.gz
af0c96a7f3f1eb528e21af366d4fd6f585dd5b6620d147822f028bb4c14023f0 guix-build-29
...
📝 Crypt-iQ opened a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956)
`CConnman::Stop()` resets `semOutbound`, yet `m_reconnections` is not cleared in `Stop`. Each `ReconnectionInfo` contains a grant member that points to the memory that `semOutbound` pointed to and `~CConnman` will attempt to access the grant field (memory that was already freed) when destroying `m_reconnections`. Fix this by calling `m_reconnections.clear()` in `CConnman::Stop()` and add appropriate annotations.
I was able to reproduce the original issue https://github.com/bitcoin/bitcoin/iss
...
(https://github.com/bitcoin/bitcoin/pull/33956)
`CConnman::Stop()` resets `semOutbound`, yet `m_reconnections` is not cleared in `Stop`. Each `ReconnectionInfo` contains a grant member that points to the memory that `semOutbound` pointed to and `~CConnman` will attempt to access the grant field (memory that was already freed) when destroying `m_reconnections`. Fix this by calling `m_reconnections.clear()` in `CConnman::Stop()` and add appropriate annotations.
I was able to reproduce the original issue https://github.com/bitcoin/bitcoin/iss
...
💬 sedited commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2566504962)
I'm confused by this change. Why are you introducing this variable? The comment is also wrong, since this processes the block on the background chain. I would still prefer if the behaviour of ProcessNewBlock would not be changed as part of this patch set.
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2566504962)
I'm confused by this change. Why are you introducing this variable? The comment is also wrong, since this processes the block on the background chain. I would still prefer if the behaviour of ProcessNewBlock would not be changed as part of this patch set.
💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#issuecomment-3583308231)
Thanks for all the feedback so far and sorry for the slow response!
> Rather than being direct RPC functionality, maybe it would be better to have an RPC function to export a copy of the utxo set at the current height, and have a separate bitcoin-kernel binary that performs the rollback and utxoset stats calculation itself?
Hm, feels like a bit overengineered for this functionality, considering the overhead for test coverage and build changes for this. Maybe I am overcomplicating it in my
...
(https://github.com/bitcoin/bitcoin/pull/33477#issuecomment-3583308231)
Thanks for all the feedback so far and sorry for the slow response!
> Rather than being direct RPC functionality, maybe it would be better to have an RPC function to export a copy of the utxo set at the current height, and have a separate bitcoin-kernel binary that performs the rollback and utxoset stats calculation itself?
Hm, feels like a bit overengineered for this functionality, considering the overhead for test coverage and build changes for this. Maybe I am overcomplicating it in my
...
💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566567639)
Changed to an `Assume` instead.
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566567639)
Changed to an `Assume` instead.
💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566567768)
I don't think so, my understanding is that the cursor/LevelDB iterator works on a snapshot of the DB itself which doesn't get mutated. You can see the same pattern in `WriteUTXOSnapshot` where we are working with a cursor without holding `cs_main` as well.
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566567768)
I don't think so, my understanding is that the cursor/LevelDB iterator works on a snapshot of the DB itself which doesn't get mutated. You can see the same pattern in `WriteUTXOSnapshot` where we are working with a cursor without holding `cs_main` as well.