💬 fanquake commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2178709604)
> Is something required to undraft this PR?

Fixing the broken Windows build. Which is now done.

👋 fanquake's pull request is ready for review: "depends: build libevent with CMake"
(https://github.com/bitcoin/bitcoin/pull/29835)

💬 m3dwards commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1646226121)
CCache is currently written to cache docker volume which I assume on Cirrus CI gets shared between multiple jobs? As a github actions is more idempotent / ephemeral this isn't going to work as the ccache dir on the host is left empty at the end of the run.

I can see that this is the same strategy used for depends output and sources and previous_releases.

The solution could be to modify `02_run_container.sh` script to have "GHA mode" where instead of using docker volumes we just bind mount
...

👍 TheCharlatan approved a pull request: "fuzz: Fix wallet_bdb_parser 32-bit unhandled fseek error"
(https://github.com/bitcoin/bitcoin/pull/30307#pullrequestreview-2128357217)
ACK fa7bc9bbca9348cf31b97bee0789ea7caeec635c

💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1646250212)
Ah right. I forgot about this. Your suggestion sounds good.

💬 maflcko commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646290588)
May be good to explain why this is needed, when `m_node.chainman->m_validation_cache` already exists?

💬 m3dwards commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178864909)
Is the plan to move everything from Cirrus to GHA?

💬 fanquake commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2178870629)
Guix Build (aarch64):
```bash
db8e2595bb6b4d7088860f1df0c952be13a4ae3db3396a0c3a42de98165529bc guix-build-a5180bffbe78/output/aarch64-linux-gnu/SHA256SUMS.part
e90d5520e9bec9efe81387012d6b9ddd189ed7f4737726d825cd8077c4c0b50b guix-build-a5180bffbe78/output/aarch64-linux-gnu/bitcoin-a5180bffbe78-aarch64-linux-gnu-debug.tar.gz
a0b5ee10285feddbc964a694c134dcd0bb9193065985423063a353c2e518442b guix-build-a5180bffbe78/output/aarch64-linux-gnu/bitcoin-a5180bffbe78-aarch64-linux-gnu.tar.gz
d49594
...

📝 luke-jr opened a pull request: "QA: Expect PACKAGE_NAME rather than constant "Bitcoin Core""
(https://github.com/bitcoin/bitcoin/pull/30308)
Followup to #29144

💬 dergoegge commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1646353890)
No need for the actual rng, we can just consume the nonce from the fuzz data.

```suggestion
CBlockHeaderAndShortTxIDs cmpctblock{*block, fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
```

💬 kevkevinpal commented on pull request "QA: Expect PACKAGE_NAME rather than constant "Bitcoin Core"":
(https://github.com/bitcoin/bitcoin/pull/30308#issuecomment-2178928580)
ACK [197b540](https://github.com/bitcoin/bitcoin/pull/30308/commits/197b5404b0f4a1d6e989000845b45c8bd24e0bc6)

we're doing this in these files aswell so seems fine to me
```
test/functional/wallet_multiwallet.py
test/functional/tool_wallet.py
test/functional/interface_bitcoin_cli.py
test/functional/feature_filelock.py
```

💬 maflcko commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178933486)
If someone finds a solution to all cache issues, then it can be done. (Moving back should be easy in any case)

For now, see the issue description:

> ideally only tasks with `NO_DEPENDS=1` are moved for now. It would be:
>
> * `ci/test/00_setup_env_native_fuzz.sh:export NO_DEPENDS=1`
>
> * `ci/test/00_setup_env_native_tidy.sh:export NO_DEPENDS=1`

💬 kevkevinpal commented on pull request "QA: Expect PACKAGE_NAME rather than constant "Bitcoin Core"":
(https://github.com/bitcoin/bitcoin/pull/30308#issuecomment-2178943305)
I see we're doing something similar in `./test/functional/wallet_backwards_compatibility.py`

```
node_v17.assert_start_raises_init_error(["-wallet=w3"], "Error: Error loading w3: Wallet requires newer version of Bitcoin Core")

and

node_v16.assert_start_raises_init_error([f"-wallet={wallet_name}"], f"Error: Error loading {wallet_name}: Wallet requires newer version of Bitcoin Core")
```

not sure if you want to update here aswell in this PR

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646370563)
I was thinking it might be useful to control the caches directly for the tests, but on second thought that does not seem like a good thing. Will revert.

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2178956138)
6ad4aa82056030a8a72b1315676e0703c1500d0d -> fa660a266b3aa03f40c47a9330e3bce8be89bb54 ([noGlobalScriptCache_4](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_4) -> [noGlobalScriptCache_5](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalScriptCache_4..noGlobalScriptCache_5))

* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1645856227), changing in
...

💬 maflcko commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646381635)
nit: You can combine the cast by using `()` over `{}`. Also you can specify the template to avoid the `size_t` cast, and ensure the right template is always picked:

```cpp
uint32_t requested_num_elems(std::min<size_t>(

💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1646397855)
@laanwj It's true that you generally shouldn't use `using namespace std;` in headers, but the C++ Core Guidelines actually have an [exception](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf7-dont-write-using-namespace-at-global-scope-in-a-header-file) for the standard literals. The reason being that literals without underscore are reserved for the standard library, so there is no risk of namespace pollution.

✅ 0xB10C closed a pull request: "rpc: introduce getversion RPC"
(https://github.com/bitcoin/bitcoin/pull/30112)

💬 0xB10C commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2178998633)
While this has a few Concept ACKs, I'm not sure the Approach (introducing a new RPC) is really the best route here. I'm closing this for now as I'll be focusing on other projects. Feel free to pick this up if you want.

💬 glozow commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1646027082)
Needs documentation, i.e. mentioning the nonce should be randomly generated