💬 ryanofsky commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654864264)
In commit "Have testBlockValidity hold cs_main instead of caller" (c8343e29a66082b37c37ee0ff8f7433b97eea9b1)

re: https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654687943

> `state` will be valid, no? So this may be confusing?

It seems like it would be better to do something like:


```c++
if (block.hashPrevBlock != tip->GetBlockHash()) {
state.Error("Block does not connect to current chain tip.")
return false;
}
```

💬 ryanofsky commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#issuecomment-2191753566)
It would be good to mention that the are some changes to cs_main locking (including a potential bugfix) in the PR description, otherwise this should be a refactoring with no changes in behavior.

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654954901)
thanks fixed in f169562d85b332576b3a1cf32755e43023227ed9

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654955419)
thank you fixed in f169562d85b332576b3a1cf32755e43023227ed9

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2191820659)
> Since calling `gettxoutsetinfo` with block hash depends on `coinstatsindex`. It would be simpler to address it on `feature_coinstatsindex`. It would be just one line, and would avoid the node restart and the function reordering.

That makes more sense I went ahead and did so in f169562d85b332576b3a1cf32755e43023227ed9

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654960188)
do you think I should still add this after f169562d85b332576b3a1cf32755e43023227ed9, rather than invalidating a block to then use `gettxoutsetinfo` with its hash I just used a random block hash. I can try doing this aswell

🚀 fanquake merged a pull request: "guix: build with glibc 2.31"
(https://github.com/bitcoin/bitcoin/pull/29987)

👍 fanquake approved a pull request: "build: Drop redundant `sys/sysctl.h` header check"
(https://github.com/bitcoin/bitcoin/pull/30327#pullrequestreview-2142044015)
ACK c0b5ea5901d0ed005bca345e5b2de8a502f6af75 - we could got the other way, and add nested #defs, but that doesn't seem worthwhile.

🚀 fanquake merged a pull request: "build: Drop redundant `sys/sysctl.h` header check"
(https://github.com/bitcoin/bitcoin/pull/30327)

💬 brunoerg commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654994309)
nit: You could use named args here. It's better to understand what `"none"` and `True` are.

👍 BrandonOdiwuor approved a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2142120592)
Code Review ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e

🤔 theuni reviewed a pull request: "guix: build with glibc 2.31"
(https://github.com/bitcoin/bitcoin/pull/29987#pullrequestreview-2142132960)
Post-merge utACK b5fc6d46a3854c18f6e8dfc89540d24ef778caa2

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2191932755)
Force push was to squash all fixups (no other changes).

💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1655069069)
sounds good resolved in [6809ffd](https://github.com/bitcoin/bitcoin/pull/30340/commits/6809ffd8a6c589c515af48da2dd8367e4c6c2c26)

💬 theuni commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655081131)
Missing:
```c++
#include <cstdint>
#include <chrono>
#include <array>
```

(Noticed while trying to forward-declare `netaddress.h`, which didn't work because of the `CService`s stored in `MappingResult`)

💬 theuni commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655098222)
This header can be made dependency-free (while making `netaddress.h` a little more forward-declare-friendly) with:
```diff
diff --git a/src/netaddress.h b/src/netaddress.h
index 52fecada1c9..75d172ab334 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -31,3 +31,3 @@
*/
-enum Network {
+enum Network : int {
/// Addresses from these networks are not publicly routable on the global Internet.
diff --git a/src/util/netif.cpp b/src/util/netif.cpp
index ad3f93b379a..c4d4c3a31c
...

💬 pinheadmz commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1655133216)
I'm actually in favor of adding `tor_port()` to `util.py` and binding every bitcoind in the functional tests to a unique port by default. It is consistent with how we set the other two ports and feels like an oversight not to assign tor port the same way, as long as it doesn't interfere with anything else.

I know what you mean that its not needed but it also default behavior to bind a tor port so I feel like we should do that in the functional tests to prevent some future regression.

💬 achow101 commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#issuecomment-2192067333)
ACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68

💬 paplorinc commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655154623)
@maflcko, so my point was that it seems to me that changing the benchmark slightly changes it's performance considerably:
```C++
static void SipHash_32b(benchmark::Bench& bench)
{
uint256 x;
uint64_t k1 = 0;
bench.run([&] {
*((uint64_t*)x.begin()) = SipHashUint256(0, ++k1, x);
});
}
```

works with inputs such as:
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/66d3022e-fa15-4950-bf12-c7db9896d766">
and the benchmark results in:
> make -j10 && .
...

💬 maflcko commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655160631)
Yes, this is expected. If you change the code that is benchmarked, the compiler will compile it to something else, which will perform differently, because it calculates something else.