💬 achow101 commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-3643539057)
ACK c1e554d3e5834a140f2a53854018499a3bfe6822

💬 mzumsande commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611870620)
I think so. But I think that `tor_port(MAX_NODES) + 1` isn't a good choice because it will lead to collisions among parallel running tests as soon as there is more than one test using the feature. I changed it to `p2p_port(n) + PORT_RANGE *3`.

🚀 achow101 merged a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414)

💬 maflcko commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611907262)
Haven't looked here in detail, but my preference would be to not call `+` or `+=` on an integral port returned by a helper function. This will lead to issues down the line, and it would be better to always call the helper that internally asserts and checks the port range. See https://github.com/bitcoin/bitcoin/pull/33260/files

💬 mzumsande commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611923052)
In trying to apply this I think I found a more general problem with port collisions:
As soon as more than one test uses this feature, there is the possibility of port collisions between two tests run in parallel. So we would need to assign separate ranges.
We know that a node will only make up to 11 outbound connections at a given moment, but currently disconnections/new connections will just be added to `self.auto_outbound_destinations`, so something like
```
def auto_listener_port(n):

...

💬 mzumsande commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611923854)
done

🤔 pablomartin4btc reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3569365873)
re-ACK d9319b06cf82664d55f255387a348135fd7f91c7

Checked the diffs since my last [review](https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3302612544).

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3643843248)
Nice. Only non-test change is adding the missing `std::move` to avoid the copy.

review ACK 07135290c1720a14c9d2f18a5700bb6565ae7a10 🏪

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N9
...

🤔 brunoerg reviewed a pull request: "Implementation of SwiftSync"
(https://github.com/bitcoin/bitcoin/pull/34004#pullrequestreview-3569068972)
I've ran a mutation analysis on this PR. Unkilled mutants should be killed (covered) by an unit or functional test (if make sense). However, I noticed that there are part of the code that doesn't even have test coverage (corecheck.dev would have show it but did not generated the report for this PR) and should have it. As soon as more tests are added to this PR, I can re-run the analysis again.

💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611914615)
We could exercise these errors (file doesn't exist and open failure) in the functional tests. An (obvious) unkilled mutant:

```diff
diff --git a/src/init.cpp b/muts-pr-34004-init-cpp/init.mutant.1.cpp
index fb7336f9ae..59de0f6b7a 100644
--- a/src/init.cpp
+++ b/muts-pr-34004-init-cpp/init.mutant.1.cpp
@@ -1812,7 +1812,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
AutoFile afile{file};
if (afile.IsNull()) {
LogError
...

💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611893305)
Unkilled mutant:
```diff
diff --git a/src/validation.cpp b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
index 3d84576e7d..efc5037570 100644
--- a/src/validation.cpp
+++ b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
@@ -2613,7 +2613,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
// * legacy (always)
// * p2sh (when P2SH enabled in flags and excludes coinbase)
// * witness (when witness enabled in flags an
...

💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611981935)
This error isn't covered by any test and could be addressed in `feature_swiftsync`. Perhaps by deleting a block file before calling the `generatetxohints` RPC.

💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611879532)
> these failures could be exercised in the provided unit test

+1. Some unkilled mutants for obvious lack of testing:

```diff
--- a/src/swiftsync.cpp
+++ b/muts-pr-34004-swiftsync-cpp/swiftsync.mutant.25.cpp
@@ -82,7 +82,7 @@ HintsfileReader::HintsfileReader(AutoFile& file) : m_file(file.release())
}
uint8_t version{};
m_file >> version;
- if (version != FILE_VERSION) {
+ if (1==0) {
throw std::ios_base::failure("HintsfileReader: Unsupported file versi
...

💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2612145025)
nit:
```suggestion
for _ in range(NUM_SWIFTSYNC_BLOCKS):
```

💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2612266788)
Unkilled mutant:
```diff
diff --git a/src/swiftsync.h b/muts-pr-34004-swiftsync-h/swiftsync.mutant.0.h
index 2c60e96895..ae229d18d8 100644
--- a/src/swiftsync.h
+++ b/muts-pr-34004-swiftsync-h/swiftsync.mutant.0.h
@@ -36,7 +36,7 @@ private:
public:
Aggregate();
/** Is the internal state zero, representing the empty set. */
- bool IsZero() const { return m_limb0 == 0 && m_limb1 == 0 && m_limb2 == 0 && m_limb3 == 0; }
+ bool IsZero() const { return m_limb0 == 0 || m_lim
...

🤔 mzumsande reviewed a pull request: "net: Waste less time in socket handling"
(https://github.com/bitcoin/bitcoin/pull/34025#pullrequestreview-3569594119)
ACK 5f5c1ea01955d277581f6c2acbeb982949088960

Looks like an improvement, but I wonder if instead of doing micro-optimizations like the ones here, it wouldn't be better to have the entire `InactivityCheck()` procedure run somewhere else than in the `SocketHandlerConnected`, or at least less often - it seems a bit absurd to check every `50ms` for a timeout of 20 minutes. Even the initial waiting time `m_peer_connect_timeout` from `ShouldRunInactivityChecks()` for a new peer doesn't really be che
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3644102756)
I've rebased due to https://github.com/bitcoin/bitcoin/pull/33602, and added several touchups. Thank you @l0rinc for the suggestions!

Thank you also @l0rinc for your very thorough measurements. I think 4 threads is a decent choice for now, but as @TheBlueMatt suggests I will try and run benchmarks in a cloud environment with network connected storage.

I would like to reproduce the memory findings, but https://github.com/bitcoin/bitcoin/issues/33351 makes it a little difficult to determine
...

💬 stickies-v commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2612421141)
nit: for people unfamiliar with these tests, it's not obvious that the relevant debug/category settings are set in `LogSetup`. would be a nice improvement to make the test more self contained by explicitly setting them at the beginning of the test (even if it's duplication)

👍 stickies-v approved a pull request: "log: Remove brittle and confusing LogPrintLevel"
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3569753904)
ACK fa35682637e7848106189e314a963d8f6c45b2bc

⚠️ memeticae opened an issue: "Bitcoin to send field is nearly invisible"
(https://github.com/bitcoin/bitcoin/issues/34052)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo

- [x] I still think this issue should be opened here

### Report

Bitcoin to send field is nearly invisible, it works, but the up and down arrows completely obfuscate the text area.

✅ pinheadmz closed an issue: "Bitcoin to send field is nearly invisible"
(https://github.com/bitcoin/bitcoin/issues/34052)