💬 ajtowns commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1581674023)
re ACK 686629d2be5545ef59cf0e97f4f3a74c6cde2efa

🤔 jarolrod reviewed a pull request: "ci: Nuke Android APK task, Use credits for tsan"
(https://github.com/bitcoin/bitcoin/pull/27834#pullrequestreview-1468894928)
NACK

I'm not sure if the PR description counts as "many" issues.

If this task was failing intermittently all the time, why are there no issues posted about these intermittent failures occurring all the time, everywhere? We watch this task often in the QML repo and have never had it fail due to a network timeout.

It's great to speed up a task, and I wish all tasks could run as quickly as possible; that would be really great. But not at the expense of a check on building for a platform we
...

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1581806840)
> * The potential downside is clear: by default, bitcoind considers both IPv4 & IPv6 to be reachable. However, both Martin & I have experienced running nodes within a network setup such that only connection attempts to IPv4 nodes were successful. A node in this environment running current master will still have failed IPv6 connection attempts. But the problem (aka frequency of failed attempts) would increase in severity if we added this patch with logic that treated IPv4 & IPv6 separately.

I'
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1222386277)
yeah, the reason for this function signature was to enable calling from within the `else if(...)` conditional. pulling it out would mean having to run `MaybePickPreferredNetwork` every time `ThreadOpenConnections` runs, which seems unnecessary?

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1222388434)
I think the requirement here is only "try each network with reasonable probability" -- you want to be connected to all of the networks, so which one comes first doesn't matter very much.

If you don't have ipv4 or ipv6 connections but are going through this codepath, then that means you have all your outbounds connected via privacy networks, but haven't chosen to `-onlynet=tor` or similar. So biassing towards clearnet probably makes sense anyway: clearnet connections probably have higher bandw
...

💬 MarcoFalke commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581873413)
> why are there no issues posted about these intermittent failures

Because no one reported them. But they do exist, if you grep for "Warning: An error occurred while preparing SDK package Android Emulator: Connection reset." For example, https://cirrus-ci.com/task/5766445213155328?logs=build#L2700

> It's great to speed up a task, and I wish all tasks could run as quickly as possible;

It only checks that it can build, and doesn't run any tests, so I hope the other tasks remain to run the
...

🤔 MarcoFalke reviewed a pull request: "fuzz: wallet, add target for `fees`"
(https://github.com/bitcoin/bitcoin/pull/27647#pullrequestreview-1468982852)
lgtm ACK 4da8d6bf17c0875c1a8f60ad2bb1bbd418d3a7cd

left some nits, didn't test

💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1222442326)
not needed?

💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1222442250)
Any reason to make this a global when it can be just a reference local in the body of `FUZZ_TARGET_INIT`?

💬 jarolrod commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581881457)
@MarcoFalke withdrew NACK, I've misunderstood intentions and outcomes of this PR. It is true that right now outside of checking for build support, this doesn't do anything for this repo.

💬 MarcoFalke commented on pull request "kernel: Remove args, settings, chainparams, chainparamsbase from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1581909843)
nit: In the last commit, any reason to use `\ ` and `\:`? Seems to pass with just ` ` and `:` for me.

lgtm ACK db77f87c6365cb5f414036d6bfb1a12705772028 🍄

<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/K
...

👍 vasild approved a pull request: "p2p: return `CSubNet` in `LookupSubNet`"
(https://github.com/bitcoin/bitcoin/pull/26078#pullrequestreview-1469207921)
ACK fb3e812277041f239b97b88689a5076796d75b9b

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1582072953)
`4c867de996...35fc849412`: address a suggestion

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1222595950)
I would address your suggestion, but I am not sure I understand it. Is the above what you meant or did I get it wrong?

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1582106400)
`fcd4d34b96...8b495486e2`: rebase due to conflicts

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1222639887)
Yeah, I think you can just copy paste it from the other test, no?

📝 MarcoFalke opened a pull request: "ci: Invalidate Cirrus CI docker cache"
(https://github.com/bitcoin/bitcoin/pull/27838)
Currently the Cirrus CI seems to fail for some reason. No idea why, but maybe invalidating the Docker image cache fixes it?

The failure is:

```
Failed to start an instance! Failed to pull null image! Repository does not exist or may require authentication.
Container errored with 'ImagePullBackOff: Back-off pulling image "gcr.io/cirrus-ci-community/bitcoin/bitcoin/ci/test_imagefile:b3e086572130d8954f84bb90778d02e2cfbb6dc624c01e2f74ee17335a9c453e"'
```

https://cirrus-ci.com/tas
...

💬 ismaelsadeeq commented on pull request "ci: Invalidate Cirrus CI docker cache":
(https://github.com/bitcoin/bitcoin/pull/27838#issuecomment-1582140841)
Concept ACK

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#issuecomment-1582165674)

> I'd really prefer we have a regtest-only option to skip the check to impact integration tests the least.

I added a support for skipping the check only on regtest in 2fec7f362d8540cda5ff7e3e4dfb2cb751364e06 by passing -acceptstaleestimates option and it's tested in dea3accb1f2eeef74f293262c168d68c0ec444cb

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1582194359)
`35fc849412...bedbdf4a15`: address https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1219413577