💬 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.

💬 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) {
```

💬 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)
...

💬 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.

⚠️ 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
...

💬 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".

💬 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
...

👍 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

📝 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.

💬 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
...

👍 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

💬 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
...

💬 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)

🤔 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.

💬 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."

💬 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.

💬 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.

💬 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

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916279728)
131eb4e630f958f1b5330bb9d170716c33655880: this should be a constant, not part of `BlockCreateOptions`.

E.g. `MINIMUM_BLOCK_RESERVED_WEIGHT{2000}` defined next to `DEFAULT_BLOCK_MAX_WEIGHT`.

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1916278786)
131eb4e630f958f1b5330bb9d170716c33655880: sane -> safety