💬 davidgumberg commented on pull request "ci: Bump centos stream 10":
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2591506347)
Concept ACK

The CentOS Stream 9 CI task as-is uses the core python3 packages which are `3.9`, below the minimum `3.10` and results in a syntax error any time `test/functional/combine_logs.py` is invoked since [it uses](https://github.com/bitcoin/bitcoin/commit/c9fb38a590e3961e68e942d71f202e357466d15f) a `match` statement.

It seems that CMake has been complaining in every single centos CI run[^1] since the minimum version was bumped to [3.10](https://github.com/bitcoin/bitcoin/pull/30527/)
...

💬 TheBlueMatt 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-2591522083)
> 1. reduces (tail) latency when requesting a block. In most cases it won't matter, but immediately after finding a new block there is potentially a lot of cs_main contention with other peers sending us copies of the block header or peers requesting the block's transactions. Even ignoring the on-block stuff, you don't want to find yourself waiting for some relatively large transaction to fetch a bunch of UTXO entries off disk to get into the mempool to build your new block!

An interface to re
...

💬 davidgumberg commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2591535568)
ACK https://github.com/bitcoin/bitcoin/pull/31651/commits/fabefd9915802d53d8c83ae66e5cb259196d9422

Ran locally and all tests passed.

```bash
env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'FILE_ENV="./ci/test/00_setup_env_native_centos.sh" ./ci/test_run_all.sh'
```

📝 theStack opened a pull request: "test: p2p: fix sending of manual INVs in tx download test"
(https://github.com/bitcoin/bitcoin/pull/31658)
The `test_inv_block` sub-test in p2p_tx_download.py has a subtle bug: the manual msg_inv announcements from peers currently have no effect, since they don't match the wtxidrelay setting (=true by default for `P2PInterface` instances) and are hence ignored by the nodes (since 2d282e0c / PR #18044):

https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/src/net_processing.cpp#L3904-L3911

Though the test still passes on master, it does so without the intended scenari
...

🤔 tdb3 reviewed a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2551472928)
Post-merge ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390

Executed functional tests while running Wireshark.
Observed no node related traffic with external addresses.
Traffic to 0.0.0.1, 0.0.0.2, and 1.2.3.4 was observed on master before this PR was merged (e.g. bb5f76ee013ee439f70e86319d22f308db8a24ea).

📝 manlovepang opened a pull request: "1"
(https://github.com/bitcoin/bitcoin/pull/31659)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915912236)
> Ah, of course. The actual problem is that we are shifting even out of the widened type's range, so it wraps around.

I see. So the max amount you can safely shift is without overflowing is not `std::numeric_limits<W>::digits - 1` but actually `std::numeric_limits<W>::digits - std::numeric_limits<T>::digits` since that is how much free space there is to the right of T's bits in W.

So maybe if you switch to that amount and change the test cases to:

TestOverflow<int8_t, int64_t>(fuzz
...

✅ manlovepang closed a pull request: "1"
(https://github.com/bitcoin/bitcoin/pull/31659)

📝 manlovepang reopened a pull request: "1"
(https://github.com/bitcoin/bitcoin/pull/31659)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 i-am-yuvi commented on pull request "test: p2p: fix sending of manual INVs in tx download test":
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2591646519)
Concept ACK

Good catch @theStack!

💬 ryanofsky 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-2591648579)
> After the notification fires, the miner calling back in to request a CreateNewBlock run will now have to wait behind `cs_main` for random P2P stuff

I don't see why this would be true. There's no reason Mining.createNewBlock or BlockTemplate.waitNext methods need to use cs_main. cs_main is an implementation detail and not part of the interface. For example we could have:

```c++
std::shared_ptr<CBlockTemplate> g_block_template;
std::mutex g_block_template_mutex;
std::condition_variable g_block
...

🤔 mzumsande reviewed a pull request: "test: p2p: fix sending of manual INVs in tx download test"
(https://github.com/bitcoin/bitcoin/pull/31658#pullrequestreview-2551617694)
Concept ACK

Good find! I think that `p2p_orphan_handling.py` has a similar [spot](https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/test/functional/p2p_orphan_handling.py#L431).

💬 i-am-yuvi commented on pull request "test: p2p: fix sending of manual INVs in tx download test":
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2591741584)
It could be good followup! Can you also change that as well @theStack

👍 i-am-yuvi approved a pull request: "test: p2p: fix sending of manual INVs in tx download test"
(https://github.com/bitcoin/bitcoin/pull/31658#pullrequestreview-2551649291)
Tested ACK e0b333682222927d64217b07bb8cfd7ff3139660

✅ fanquake closed a pull request: "1"
(https://github.com/bitcoin/bitcoin/pull/31659)

📝 fanquake locked a pull request: "1"
(https://github.com/bitcoin/bitcoin/pull/31659)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 maflcko commented on pull request "test: p2p: fix sending of manual INVs in tx download test":
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2591895415)
> It might be good to avoid issues like this in the future, happy to add test framework improvements if someone has a concrete idea.

Would it not be possible to use mocktime and sync_with_ping?

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916121562)
Yeah, that should work. I'll fuzz with this for some time.

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

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