👍 rkrux approved a pull request: "test: enhance p2p_orphan_handling"
(https://github.com/bitcoin/bitcoin/pull/31037#pullrequestreview-2400959381)
tACK 9de9c858d5aa412e1c6086e564fbe85984a4f2d5

Successful make and all functional tests. This is my preference as well to use the `getorphantxs` RPC for asserting over searching in the debug logs.

💬 laanwj commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2443481677)
> isn't really something we need to investigate further / work on reintroducing.

Agree, it's fine, it's good to get rid of the perfmon query.

💬 maflcko commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1820289265)
nit: For new code it would be good to use the alias `chainman().GetMutex()` member instead, when `chainman()` members are accessed.

💬 jarolrod commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2443594716)
Concept ACK

💬 jarolrod commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2443600588)
Concept ACK

💬 jarolrod commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2443601472)
Concept ACK

🤔 jarolrod reviewed a pull request: "Decouple WalletModel from RPCExecutor"
(https://github.com/bitcoin-core/gui/pull/841#pullrequestreview-2401161662)
strong concept ack

💬 jarolrod commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443649838)
> but merely to give a feeling and to avoid a regression where an algorithm is swapped to go from O(n) to O(n^2). So in a sense, they are a bit of a unit test.

This is a good reason to keep.

> I'm arguing for scoping our tests to the latter (and other critical things e.g. mining) so that we don't get distracted.

And this is an important statement.

We don't necessarily need to throw away work that has previously been a useful metric in issues (https://github.com/bitcoin/bitcoin/issues
...

💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2443665126)
> Looks like that fixed it. However, it would be good to understand _why_ it fixed it. Looking at the diff that triggered it, I fail to see how ([#31000 (comment)](https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680))

Running on another branch with `-testdatadir=foo` I get files like:
`foo/test_common bitcoin/validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch/datadir/regtest/chainstate_snapshot_INVALID/MANIFEST-000004`

Combined with `C
...

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2443678171)
> Running on another branch with `-testdatadir=foo`

The Windows CI doesn't set the flag and the push/diff I linked to didn't change the non `-testdatadir` code path, but the previous commit passed and the next one failed. Just asking to understand what is going on with the CI.

💬 hodlinator commented on pull request "util: Remove RandAddSeedPerfmon":
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2443686717)
> The high quality OS entropy source on windows is `CryptGenRandom`, and we use that.

[A Microsoft article](https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom) from 2021 on `CryptGenRandom` states:
> **Important** This API is deprecated. New and existing software should start using [Cryptography Next Generation APIs.](https://learn.microsoft.com/en-us/windows/desktop/SecCNG/cng-portal) Microsoft may remove this API in future releases.

So maybe we coul
...

💬 laanwj commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1820458290)
Maybe add something like "if the IDE has an option for this" to make it slightly more clear, out of context, that it's an IDE option, and not something to set with say, `debugedit`.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820474026)
Interesting. So, for me, the first commit (with a new test) takes forever to execute.

More specifically, the execution hangs on `best_peers.resize(targets_size);`, with `targets_size` being a huge number.

I guess your system handles this gracefully instead?

A clear failure could be triggered by e.g. `Assume(targets_size <= m_states.size());`

💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443766239)
> throw away work that has previously been a useful metric in issues (https://github.com/bitcoin/bitcoin/issues/17501)

The benchmarks weren't mentioned in the issue you linked or the PR that fixed the issue?

💬 fanquake commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2443772429)
Generally I would prefer if we weren't making source code changes to fix (currently edge-case) Windows linking errors; especially if there are more complete fixes we could be making (even if later-on).

It'd be good if all commits explained the problem, and fix. i.e the first commit 4a1accd1df31cba778167b6a3ff9783ffedb46ab `refactor: Inline cs_main definition` has no explanation of what the problem/fix is, and doesn't mention that the change is only needed to fix a build issue when using MSVC,
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820487554)
4234f5ebe133b949080aaa56da8cbdd18b650ff2

Originally, the code forced me to make this call for every peer, that's why i needed it to be deterministic. It's not the case anymore. Do we still need determinism then?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820490053)
Otherwise we can drop `m_deterministic_randomizer` altogether and use some cheap `FastRandomContext` instead.

💬 itornaza commented on pull request "consensus: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2443779806)
Hey @Christewart! Do you plan to update for cmake?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820518288)
There is no need to cast it to double anymore.

What we should do is something like
```
size_t outbounds_target_size = 0;
if (OUTBOUND_FANOUT_DESTINATIONS > outbounds_fanout_tx_relay) outbounds_target_size = OUTBOUND_FANOUT_DESTINATIONS - outbounds_fanout_tx_relay;
```

Perhaps there is a less ugly way to do this. But casting to double seems worse.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820521627)
feel free to resolve this and let's talk in the new commit.