💬 rkrux commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1827276868)
The PR title states that this will enable running functional test sub-tests but any method can be run with this argument at the moment such as one below. Should we restrict only for those that have the `test_` prefix?

```
+ def sample_calculator(self):
+ self.log.info("Sample calculator running")
+ x = 2 + 3
+ self.log.info(f'Calculated x to be: {x}')
+
```

💬 rkrux commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1827273054)
Since the argument is a list named `test_methods`, let's call this function `run_custom_tests`?

💬 rkrux commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1827274569)
Might as well find the searchable & callable methods initially and execute them while throwing the error for those that are filtered out? Otherwise few would be executed and the first incorrect one would throw error.

💬 maflcko commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454015922)
Can you motivate this a bit better? `getInt` is used in many RPCs. What makes this call to `getInt` in this RPC special? What real-world end-user-facing issue is this trying to solve?

💬 maflcko commented on issue "guix: Linux and macOS builds are not cross-arch reproducible with powerpc64le build arch":
(https://github.com/bitcoin/bitcoin/issues/31207#issuecomment-2454024773)
> I would be happy to upload my binaries and/or run diffoscope on request. I would also be happy to run Guix builds of additional commits on my powerpc64le machine if there are commits that seem likely to be the culprit.

I'd say all of that would be useful. You link to the GCC bump to 12, which seems one of the more significant changes to the Linux build, so testing commit e1ce5b8ae9124717c00dca71a5c5b43a7f5ad177 (and `e1ce5b8ae9124717c00dca71a5c5b43a7f5ad177~1`), or otherwise bisecting could
...

💬 maflcko commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2454033675)
review ACK 42066f45ff5d48e78a317eda63c035809bd657c6

Didn't re-check ubsan

💬 maflcko commented on pull request "doc: Use relative hyperlinks in release-process.md":
(https://github.com/bitcoin/bitcoin/pull/31206#issuecomment-2454043965)
review ACK 9f71cff6ab32c04149924699a68f30eebf25955b

mlc also checks relative links, according to the link you provided, so might as well use relative ones, but no strong opinion.

💬 maflcko commented on pull request "test: Fix intermittent issue in wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/29982#issuecomment-2454049667)
lgtm as a temporary workaround, but I think the underlying issue should still be fixed

💬 hodlinator commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2454107063)
And vcpkg has a global index similar to Rust crates etc: https://vcpkg.io/en/package/libqrencode. That was the piece I was missing from the puzzle.

Thanks for bearing with me! Haven't knowingly used vcpkg for Windows dev before.

👍 hodlinator approved a pull request: "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC"
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2412505962)
ACK c9baf41372b06e08976a3daf459468d3f6ad28ea

Great to have more fully-fledged builds by default when building on Windows.

CMake & vcpkg novice here but can't spot anything funky going on. Guess one attack vector is modifying the global vcpkg index to make QREncode contain malicious code, but that's implied in vcpkg builds. Official Windows releases are cross-compiled from Linux without using vcpkg, so this risk should mostly affect devs.

Should probably remove this line from the docs:

...

💬 josibake commented on issue "Faster way to get block with prevouts in JSON-RPC":
(https://github.com/bitcoin/bitcoin/issues/30495#issuecomment-2454144977)
I think there are two separate topics here:

1. "I need to process the entire blockchain for [an external application like electrs, data analysis, etc]"
2. We can probably make the JSON-RPC faster, via threading, batching, etc

For 1., @vostrnad have you seen #30595 ? For the specific ask of prevouts, I'm almost certain this will always be faster since the the kernel API provides the prevouts by reading the rev.dat files (admittedly, I haven't looked into how this is done with the `getbloc
...

✅ maflcko closed a pull request: "test: use context managers and add file existence checks in feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31030)

💬 maflcko commented on pull request "test: use context managers and add file existence checks in feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/31030#issuecomment-2454151996)
Closing for now, due to lack of progress. If you still want to work on this, please leave a comment to have it re-opened. Alternatively, you can open a new pull request with the outstanding feedback addressed.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827403107)
Yeah just dropping `HasCollision`, keeping `HasCollisionInternal`. The former is used only in tests. And since its implementation is trivial, i think testing for collisions through `AddToSet` is sufficient.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827404289)
Right, please resolve this issue.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827406620)
My impression was that there are very-very few `reviewers already familiar with the original approach` at the moment, so it's an extra burden for most to understand that former code you drop anyway.
But yeah it's ok. Perhaps a commit message saying it will be dropped will work.

💬 fanquake commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#issuecomment-2454215099)
> Additionally, the `cmake` has been removed from the required packages,
as it is no longer specific to depends.

If `cmake` is needed to build depends it should remain listed. The same as `make`, or a compiler.

🤔 hodlinator reviewed a pull request: "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues"
(https://github.com/bitcoin/bitcoin/pull/30349#pullrequestreview-2412639608)
Concept ACK 42066f45ff5d48e78a317eda63c035809bd657c6

More realistic to also change `k0` and `val`. (The modification of the `uint256 val` only happens for one byte at a time, so benchmark time shouldn't be that affected by some unrelated expensive instructions / memory access :+1: ).

### UBSAN Repro

Managed to repro [maflcko's concern](https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1824325029) on older push c88b1bde7500ace70a694f36a82521f92221936c:
```
₿ UBSAN_OPTIONS="supp
...

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

Duplicates the comment around `MAX_RECONSET_SIZE`. One of them we better dop.

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

"Would fail" is very confusing. "Failed reconciliation" will have a technical meaning in the latter commits. Here, i'd rather be more explicit "two transactions mapped to the same short-id won't be announced between these two peers".