💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2591928909)
`bbfc58a0af...69e076664d`: drop the tests changes from this PR because they were already merged via https://github.com/bitcoin/bitcoin/pull/31646. So here remains a CI change to detect future regressions.
If people want to run this manually, outside of the CI in a way that causes false positive (discussion above), what about something like this:
```diff
--- i/ci/test/03_test_script.sh
+++ w/ci/test/03_test_script.sh
@@ -184,13 +184,15 @@ function traffic_monitor_end()
# "permissio
...
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2591928909)
`bbfc58a0af...69e076664d`: drop the tests changes from this PR because they were already merged via https://github.com/bitcoin/bitcoin/pull/31646. So here remains a CI change to detect future regressions.
If people want to run this manually, outside of the CI in a way that causes false positive (discussion above), what about something like this:
```diff
--- i/ci/test/03_test_script.sh
+++ w/ci/test/03_test_script.sh
@@ -184,13 +184,15 @@ function traffic_monitor_end()
# "permissio
...
💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2591934116)
> Having the Mining interface should only make it easier to implement support for other protocols because they can use it without needing to interact with validation code.
Which can be seen in action in RPC code that calls these methods.
>> An interface that pushes the new work itself, rather than a notification, would not have this issue.
> `waitNext` is providing an interface that pushes the new work itself. A reference to the new block is in the return value.
@TheBlueMatt when you opened
...
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2591934116)
> Having the Mining interface should only make it easier to implement support for other protocols because they can use it without needing to interact with validation code.
Which can be seen in action in RPC code that calls these methods.
>> An interface that pushes the new work itself, rather than a notification, would not have this issue.
> `waitNext` is providing an interface that pushes the new work itself. A reference to the new block is in the return value.
@TheBlueMatt when you opened
...
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1916158555)
In testnet3 and testnet4 the difficulty depends on the timestamp. After 20 minutes it drops to 1. These methods derive difficulty using the current timestamp.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1916158555)
In testnet3 and testnet4 the difficulty depends on the timestamp. After 20 minutes it drops to 1. These methods derive difficulty using the current timestamp.
💬 stratospher commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1914320847)
528b8cc: this would be more accurate for descendants.
```suggestion
} else if (pindexFirstInvalid != pindex) {
```
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1914320847)
528b8cc: this would be more accurate for descendants.
```suggestion
} else if (pindexFirstInvalid != pindex) {
```
💬 stratospher commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1914366243)
7f88af8b: when `invalidateblock` is called multiple times, we would need to clear the `BLOCK_FAILED_VALID` from previous iteration so that both `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` don't happen together on the same block. ex: [this situation](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/test/functional/rpc_invalidateblock.py#L91)
```
if (it->second->nStatus & BLOCK_FAILED_VALID) {
it->second->nStatus = (it->second->nStatus ^ BLOCK_FAILED_VALID)
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1914366243)
7f88af8b: when `invalidateblock` is called multiple times, we would need to clear the `BLOCK_FAILED_VALID` from previous iteration so that both `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` don't happen together on the same block. ex: [this situation](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/test/functional/rpc_invalidateblock.py#L91)
```
if (it->second->nStatus & BLOCK_FAILED_VALID) {
it->second->nStatus = (it->second->nStatus ^ BLOCK_FAILED_VALID)
...
💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1916173716)
I'd say that test-only docs shouldn't be put in the real RPC docs for mainnet. If someone is running on t3/t4 they should be aware of the rules already. And if they are not aware, "if found now" doesn't help to explain the rules to them.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1916173716)
I'd say that test-only docs shouldn't be put in the real RPC docs for mainnet. If someone is running on t3/t4 they should be aware of the rules already. And if they are not aware, "if found now" doesn't help to explain the rules to them.
⚠️ fxcoudert opened an issue: "The 28.1 release is not tagged in github"
(https://github.com/bitcoin/bitcoin/issues/31660)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
In GitHub releases, 28.0 is still marked as latest. We are holding from updating in Homebrew until this is resolved: https://github.com/Homebrew/homebrew-core/pull/204066
### Expected behaviour
28.1 should be marked as latest release, to confirm it is authentic.
### Steps to reproduce
1. Go to https://github.com/bitcoin/bitcoin
2. Observe that latest release is 28.0
### Relevant log o
...
(https://github.com/bitcoin/bitcoin/issues/31660)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
In GitHub releases, 28.0 is still marked as latest. We are holding from updating in Homebrew until this is resolved: https://github.com/Homebrew/homebrew-core/pull/204066
### Expected behaviour
28.1 should be marked as latest release, to confirm it is authentic.
### Steps to reproduce
1. Go to https://github.com/bitcoin/bitcoin
2. Observe that latest release is 28.0
### Relevant log o
...
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1916184067)
Ok, I dropped "if found now".
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1916184067)
Ok, I dropped "if found now".
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2592035361)
Updated 922aa7a25650bdfd8428198f171f88915af1ffa0 -> 430d5feee9b8be3a85e85696c449753783301c2b ([kernel_cache_sizes_15](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_15) -> [kernel_cache_sizes_16](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_16), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_15..kernel_cache_sizes_16))
* Addressed @ryanofsky's [comment_1](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915387311), [c
...
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2592035361)
Updated 922aa7a25650bdfd8428198f171f88915af1ffa0 -> 430d5feee9b8be3a85e85696c449753783301c2b ([kernel_cache_sizes_15](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_15) -> [kernel_cache_sizes_16](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_16), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_15..kernel_cache_sizes_16))
* Addressed @ryanofsky's [comment_1](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915387311), [c
...
👍 laanwj approved a pull request: "Cleanups to port mapping module post UPnP drop"
(https://github.com/bitcoin/bitcoin/pull/31157#pullrequestreview-2551977080)
Code review ACK 70398ae05bc36a2788e87f67ae06962f43fe35a6
(https://github.com/bitcoin/bitcoin/pull/31157#pullrequestreview-2551977080)
Code review ACK 70398ae05bc36a2788e87f67ae06962f43fe35a6
📝 hebasto opened a pull request: "depends: Always set `CMAKE_BUILD_TYPE=None` globally"
(https://github.com/bitcoin/bitcoin/pull/31661)
This PR fixes a regression for the `libevent` package introduced in https://github.com/bitcoin/bitcoin/pull/29835.
Instead of configuring `CMAKE_BUILD_TYPE=None` per package, set it globally to ensure consistent behaviour. This prevents packages with a default build type (usually "Release") from overriding our build type-specific flags, such as `-O2` or `-O1 -g`.
For packages that do not set a default build type, this change does not affect their behaviour.
(https://github.com/bitcoin/bitcoin/pull/31661)
This PR fixes a regression for the `libevent` package introduced in https://github.com/bitcoin/bitcoin/pull/29835.
Instead of configuring `CMAKE_BUILD_TYPE=None` per package, set it globally to ensure consistent behaviour. This prevents packages with a default build type (usually "Release") from overriding our build type-specific flags, such as `-O2` or `-O1 -g`.
For packages that do not set a default build type, this change does not affect their behaviour.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2592154041)
The first commit a7d43645ea353021b2143407ab06336105c82434 changes `coinbaseMaxAdditionalWeight` in the Mining interface to `blockReservedWeight`. This decouples it from the Stratum v2 concept of `CoinbaseOutputDataSize` which is strictly limited to the outputs added:
> The maximum additional serialized bytes which the pool will add in coinbase transaction outputs
https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputdatasize-client---se
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2592154041)
The first commit a7d43645ea353021b2143407ab06336105c82434 changes `coinbaseMaxAdditionalWeight` in the Mining interface to `blockReservedWeight`. This decouples it from the Stratum v2 concept of `CoinbaseOutputDataSize` which is strictly limited to the outputs added:
> The maximum additional serialized bytes which the pool will add in coinbase transaction outputs
https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputdatasize-client---se
...
👍 maflcko approved a pull request: "ci: Supply `--platform` argument to `docker build`."
(https://github.com/bitcoin/bitcoin/pull/31657#pullrequestreview-2552091496)
lgtm, but I left some feedback
(https://github.com/bitcoin/bitcoin/pull/31657#pullrequestreview-2552091496)
lgtm, but I left some feedback
💬 maflcko commented on pull request "ci: Supply `--platform` argument to `docker build`.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1916282815)
Does this env var work for podman as well? Regardless, I am wondering if it was easier to just pass `--platform` to `docker build` in `./ci/test/02_run_container.sh` (and possibly `docker run`, if needed)?
Something like:
```bash
CI_IMAGE_PLATFORM="" # native platform (default)
```
Alternatively, with the added benefit of possibly supporting macos (see https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493612110 (expand "docker setup")):
```bash
CI_IMAGE_PLATFORM="--os
...
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1916282815)
Does this env var work for podman as well? Regardless, I am wondering if it was easier to just pass `--platform` to `docker build` in `./ci/test/02_run_container.sh` (and possibly `docker run`, if needed)?
Something like:
```bash
CI_IMAGE_PLATFORM="" # native platform (default)
```
Alternatively, with the added benefit of possibly supporting macos (see https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493612110 (expand "docker setup")):
```bash
CI_IMAGE_PLATFORM="--os
...
💬 maflcko commented on pull request "ci: Supply `--platform` argument to `docker build`.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1916269793)
```suggestion
export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"
```
nit: I think you can now remove the redundant arch here? (same for the others)
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1916269793)
```suggestion
export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"
```
nit: I think you can now remove the redundant arch here? (same for the others)
🤔 Sjors reviewed a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2552083345)
Code review ad1bc03245181b00a25ea0182373eddae1c151e1, mostly happy.
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2552083345)
Code review ad1bc03245181b00a25ea0182373eddae1c151e1, mostly happy.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916285431)
I would leave out this sentence, or say "Setting a lower value is prevented at startup."
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916285431)
I would leave out this sentence, or say "Setting a lower value is prevented at startup."
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916264865)
a7d43645ea353021b2143407ab06336105c82434: you can leave out "(scriptSig, witness and outputs)" as well as "This must include ...", they are left-overs from when this reflected `CoinbaseOutputDataSize`, see https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2592154041
Suggested text:
> The default reserved weight for the block header, transaction count and coinbase transaction.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916264865)
a7d43645ea353021b2143407ab06336105c82434: you can leave out "(scriptSig, witness and outputs)" as well as "This must include ...", they are left-overs from when this reflected `CoinbaseOutputDataSize`, see https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2592154041
Suggested text:
> The default reserved weight for the block header, transaction count and coinbase transaction.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916290653)
Maybe add:
> This accounts for the block header, var_int encoding of the transaction count and a minimally viable coinbase transaction. It adds an additional safety margin, because even with a thorough understanding of block serialization, it's easy to make a costly mistake when trying to squeeze every last byte.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916290653)
Maybe add:
> This accounts for the block header, var_int encoding of the transaction count and a minimally viable coinbase transaction. It adds an additional safety margin, because even with a thorough understanding of block serialization, it's easy to make a costly mistake when trying to squeeze every last byte.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916296390)
ad1bc03245181b00a25ea0182373eddae1c151e1: the header and
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916296390)
ad1bc03245181b00a25ea0182373eddae1c151e1: the header and