💬 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.
💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566567985)
Hm, I don't think we log anything on the creation of the DB so I think I would keep it the same on the destruction. It should only be a debug level log if we would add anything like that since it seems a bit low level for the general user.
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566567985)
Hm, I don't think we log anything on the creation of the DB so I think I would keep it the same on the destruction. It should only be a debug level log if we would add anything like that since it seems a bit low level for the general user.
💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566569186)
Hm, this one and the other similar comments about test coverage are all cases that are pretty hard to hit because they should only be possible in a case of db corruption or a similarly unlikely event. Not saying that this wouldn't be valuable coverage, but afaik we hardly ever go through the hassle to cover such cases and this RPC is far from being a critical path in the comparison to the rest of the code base. So, unless you have a specific suggestion for how the can be hit in practical way I w
...
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566569186)
Hm, this one and the other similar comments about test coverage are all cases that are pretty hard to hit because they should only be possible in a case of db corruption or a similarly unlikely event. Not saying that this wouldn't be valuable coverage, but afaik we hardly ever go through the hassle to cover such cases and this RPC is far from being a critical path in the comparison to the rest of the code base. So, unless you have a specific suggestion for how the can be hit in practical way I w
...
💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566569370)
Taken
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566569370)
Taken
💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566569595)
I don't think this is necessary, I don't think a user would naturally assume that there is a reason to suspend network activity for this. Previously we had to do this as basically a hack. When we don't do this anymore it would seem odd to me to mention it.
(https://github.com/bitcoin/bitcoin/pull/33477#discussion_r2566569595)
I don't think this is necessary, I don't think a user would naturally assume that there is a reason to suspend network activity for this. Previously we had to do this as basically a hack. When we don't do this anymore it would seem odd to me to mention it.