💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1378088543)
Sure. This is because we execute the stale check every 10 min, so needed to push the last stale check time forward.
Will re-order the test checks so this becomes clearer.

💬 mzumsande commented on issue "Evicting and filling attack for linking multiple network addresses":
(https://github.com/bitcoin/bitcoin/issues/28760#issuecomment-1787932314)
I may be too pessimistic but think that preventing fingerprinting as a whole is close to a lost cause currently - not in theory, but in the sense that solving all possible fingerprinting vectors would be **a lot** of work.

I think there are fingerprinting vectors in all three relay types (addrs, transactions, blocks) - some are public, e.g. #24571, some maybe aren't documented but are pretty obvious (probably not helpful to document specifics in one public place, so I won't do that here) - an
...

🤔 furszy reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1707131031)
Updated per feedback, thanks @andrewtoth.

* Re-ordered test checks for improved clarity.
* Included coverage for when the node is stale but still recoverable (below the limited peers threshold).

🤔 jonatack reviewed a pull request: "Do the SOCKS5 handshake reliably"
(https://github.com/bitcoin/bitcoin/pull/28649#pullrequestreview-1707129039)
ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78

💬 jonatack commented on pull request "Do the SOCKS5 handshake reliably":
(https://github.com/bitcoin/bitcoin/pull/28649#discussion_r1378097032)
<details><summary>The first commit could be smaller and simpler/clearer (feel free to ignore).</summary><p>

```diff
@@ -283,13 +283,6 @@ void Sock::SendComplete(Span<const unsigned char> data,
}
}

-void Sock::SendComplete(Span<const char> data,
- std::chrono::milliseconds timeout,
- CThreadInterrupt& interrupt) const
-{
- SendComplete(MakeUCharSpan(data), timeout, interrupt);
-}
-
std::string Sock::RecvUntilTerminator(uint8
...

👍 hebasto approved a pull request: "[26.x] Backports for rc2"
(https://github.com/bitcoin/bitcoin/pull/28754#pullrequestreview-1707143194)
ACK 0989d00b7b298bef9e1faf04c1753a024b7dfd9e, I've applied backport locally and got a zero diff. Also I've reviewed the last two commits.

👍 TheCharlatan approved a pull request: "[26.x] Backports for rc2"
(https://github.com/bitcoin/bitcoin/pull/28754#pullrequestreview-1707161659)
lgtm ACK 9c29048dac5dfa5a06323b350156b1212b87d56d

💬 maflcko commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1787970115)
No idea what is wrong with Microsoft/Azure/GHA again. Locally it works fine, but not on their infra:

https://github.com/bitcoin/bitcoin/actions/runs/6711963492/job/18240509895?pr=28349#step:6:210

```
checking whether clang is Clang... yes
checking whether pthreads work with "-pthread" and "-lpthread"... yes
checking whether Clang needs flag to prevent "argument unused" warning when linking with -pthread... no
checking for joinable pthread attribute... PTHREAD_CREATE_JOINABLE
checking
...

💬 theuni commented on pull request "build: Windows SSP roundup":
(https://github.com/bitcoin/bitcoin/pull/28461#issuecomment-1787983662)
Concept ACK
[This comment](https://github.com/mesonbuild/meson/issues/10673#issuecomment-1243061073 ) (and the rest of that thread) helped me to understand what was going on here. As I understand it, "-lssp" should've never been needed. It was used as a hack for years, but is no longer required after mingw-w64 v11.

I'm confused like @fanquake though. I would have expected us to need to carry the hack for a while, considering that the fix is from this year.

💬 mzumsande commented on pull request "addrman: log AS only when using asmap":
(https://github.com/bitcoin/bitcoin/pull/28729#issuecomment-1788025388)
Code Review ACK 02a4f1a3859ed7e865641b35ca1bc9ce711e696f

💬 jonatack commented on issue "Evicting and filling attack for linking multiple network addresses":
(https://github.com/bitcoin/bitcoin/issues/28760#issuecomment-1788045181)
Introducing some randomness into the eviction algorithm to make it less deterministic -- instead of, or in addition to, random disconnection delay -- might be a potentially low-hanging countermeasure.

*"Bitcoin allows multiple connections from one single address (Saad et al. [2021](https://cybersecurity.springeropen.com/articles/10.1186/s42400-023-00182-9#ref-CR45)). This property significantly reduces the attack cost."*

#28248 proposes to improve or address this in several ways.

*"We d
...

💬 pablomartin4btc commented on pull request "gui: fix crash on selecting "Mask values" in transaction view":
(https://github.com/bitcoin-core/gui/pull/774#issuecomment-1788048860)
I do agree with @theStack backporting (checked it with the `git tag` command) since the issue technically is the "conflict" between these 2 lines:
https://github.com/bitcoin-core/gui/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/qt/transactionview.cpp#L532-L533

The last one (#L533) was added by the #708 (merged in v25 - I wanted to say I'm innocent but no haha), so another way to solve this issue is just by removing #L532, which I've tested and works fine.

💬 jonatack commented on pull request "ci: run test_bitcoin with DEBUG_LOG_OUT in RUN_UNIT_TESTS_SEQUENTIAL":
(https://github.com/bitcoin/bitcoin/pull/28736#issuecomment-1788065706)
Concept ACK.

(Maybe unrelated, but the Win64 CI task is one I've often wished for more debug info from when a unit test fails.)

💬 kevkevinpal commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1378227825)
why do we only have the first few digits here, I tried with `046d6b657901000000` and it worked fine

`assert_equal(all([not line == "046d6b657901000000" for line in f]), True)`

💬 jonatack commented on pull request "build: remove duplicate `-lminiupnpc` linking":
(https://github.com/bitcoin/bitcoin/pull/28755#issuecomment-1788174027)
ACK b74e449ffa9965f18037f0322ea178e2fe1dbc18

Build output diff is the same as my previous https://github.com/bitcoin/bitcoin/pull/28755#pullrequestreview-1705089533, and with the latest push also see a similar improvement as reported by @hebasto in https://github.com/bitcoin/bitcoin/pull/28755#pullrequestreview-1706070279:

master
```
checking for miniupnpc/miniupnpc.h... yes
checking for upnpDiscover in -lminiupnpc... yes
checking for miniupnpc/upnpcommands.h... yes
checking for upnpD
...

💬 ajtowns commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1378307864)
Could alternatively make these member functions of `Package` ?

👍 pablomartin4btc approved a pull request: "[do not merge] validation: assumeutxo params mainnet"
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-1707472149)
tACK 0800be24bc6de4154653be75b8fa9902a4a8c3a7

```
./contrib/devtools/utxo_snapshot.sh 800000 - ./src/bitcoin-cli -datadir=${AU_DATADIR}
Rewinding chain back to height 800000 (by invalidating 00000000000000000000e26b239cf19ec7ace5edd9694d51a3f6933247720947); this may take a while
Generating txoutset info...
{
"height": 800000,
"bestblock": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
"txouts": 111535121,
"bogosize": 8443325590,
"hash_serialized_3": "6e
...

💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-1788285499)
I would expect that a wallet that connects to us over (untrusted) p2p will get the exact same info regardless of background validation progress.

💬 Sjors commented on pull request "[do not merge] validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-1788287616)
@pablomartin4btc thanks for checking that you get the same snapshot. In addition it would be good to test if you're able to load the snapshot and complete the background sync.

💬 achow101 commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1378328199)
This prefix matches all possible `mkey` records so it can make sure that there are no `mkey`s slipping in elsewhere.