💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1672417787)
fixed, thanks!

💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1672452850)
Ok, I see, the `peer.wait_for_disconnect()` immediately returns and doesn't do anything.

💬 kevkevinpal commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#issuecomment-2220744320)
ACK [53f4329](https://github.com/bitcoin/bitcoin/pull/30420/commits/53f43293875de086cd0bd424fd582bc232bec3a3)

I tried increasing the time out to 6 seconds and that is when I started getting the error `OSError: Not connected`

💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1672462819)
My recommendation would be to remove `peer.wait_for_disconnect()`, since it doesn't do anything, and instead add an assert after the `assert_debug_log` scope to check the peer is disconnected.

💬 dergoegge commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2220756509)
> Niklas, everything you're saying may be true but it doesn't seem self evident.

Certainly! I think most of what I (and Matt) said are opinions and not ground truths. It does look like most of the actual discussion is philosophical in nature (software maintenance, willingness of users to run more than one piece of software, ...) and the eventual route we go will depend on a judgement call, which is why I find it important that more contributors with opinions chime in.

> I could imagine the
...

⚠️ Vero7979 opened an issue: "npm install json-payment-protocol"
(https://github.com/bitcoin/bitcoin/issues/30422)
Pay with BitPay at INFINITI of San Jose Dealership Info

Phone Numbers:
Main Line:
(408) 531-3000
Sales Direct:
(408) 531-3033
Service Direct:
(408) 531-3035
Parts Direct:
(408) 531-3032
INFINITI Roadside Assistance:
(800) 662-6200
Main:
4085313000
Sales Hours:
Mon - Sat
9:00 AM - 8:00 PM
Sun
10:00 AM - 7:00 PM
Service Hours:
Mon - Fri
7:30 AM - 6:00 PM
Sat
7:30 AM - 5:00 PM

✅ pinheadmz closed an issue: "npm install json-payment-protocol"
(https://github.com/bitcoin/bitcoin/issues/30422)

:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/30422)

👍 hebasto approved a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415#pullrequestreview-2169500423)
ACK 5a8b9432cde11f6140855717af195d8b7e798d75, tested on Ubuntu 24.04/

💬 hebasto commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1672501712)
nit: Maybe make a reference to a PR consistent with the [previous one](https://github.com/bitcoin/bitcoin/blob/9adebe145557ef410964dd48a02f3d239f488cd0/contrib/devtools/check-deps.sh#L55)?

💬 rishkwal commented on issue "utils: add support for `bitcoin-wallet` tool configuration file":
(https://github.com/bitcoin/bitcoin/issues/30421#issuecomment-2220813745)
I see `bitcoin-wallet` as a distinct entity from `bitcoind`, `bitcoin-cli`, and `bitcoin-qt`, which are tightly coupled. `bitcoin-cli` already manages wallets alongside the node, but `bitcoin-wallet` offers independent wallet management. Having a separate `wallet.conf` allows for independent wallet management without altering node settings. It provides the flexibility to manage additional wallets without affecting the main wallet configured via bitcoin-cli and simplifies configuration for users
...

🤔 hebasto reviewed a pull request: "rpc: Use untranslated error strings in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/30395#pullrequestreview-2169539317)
Post-merge ACK fa5b8920be041380fbfa4c7b443918637423d7a0.

💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#discussion_r1672545632)
> nit in [e78cf83](https://github.com/bitcoin/bitcoin/commit/e78cf83beec2f508823803947bdc3d9bcb7c7217): Not sure why nbits matter for this test.

Yeah, I had just reused the helper method from this test but it's not needed indeed. I am using an empty constructor now.

> I think you can just write `CBlockIndex block{.nChainTx=..max()};`, replacing the first two lines.

I am unfamiliar with the `..max()` syntax and I am also getting a compiler error when I try to use a designated initializer
...

💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2220876912)
> I think you can drop the new name from the pull description. It is wrong right now (doesn't match the scripted diff), and if anyone wanted to find it, they can look at the scripted diff.

done

💬 hodlinator commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220906382)
> So going forward, I guess one can only assume to surely reproduce a fuzz crash by using the exact same stdlib (and compiler). For context there is also a compiler behavior mismatch discussed in #29043.

This is a strong counter-point. I guess other stdlib-differences like `std::unordered_map` implementations already made this true though.

💬 sipa commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220914013)
I don't think there are *any* `std::shuffle` in the fuzz tests, though?

It may make sense to add something like the old `Shuffle` back if we do need it, though.

💬 maflcko commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220931147)
> I don't think there are _any_ `std::shuffle` in the fuzz tests, though?

I hope all code touched in this pull request is covered by fuzz tests. If not, then that should be fixed ;)

For example, `joinpsbts` is an RPC (not in the fuzz dir), but it is fuzzed, and it calls `std::shuffle`: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/rpc/rawtransaction.cpp.gcov.html#L1793

I don't have a strong feeling either way, because if an effort is made to have uniform fuzz behavior across compi
...

📝 fanquake opened a pull request: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423)
The current `test-security-check` script is hard to understand, and change (i.e https://github.com/bitcoin/bitcoin/pull/29987/files#diff-52aa0cda44721f089e53b128cb1232a876006ef257b211655456b17dfb2ec712); tests are also not done in isolation (when-possible). Fix that, and add missing checks. Simplifies future toolchain/security/hardening changes.

👍 hebasto approved a pull request: "util: Catch translation string errors at compile time"
(https://github.com/bitcoin/bitcoin/pull/30383#pullrequestreview-2169704590)
ACK fa601ab9f7142cdb18c18c1128fc893cdffb3463.

nit: The error messages for the cases `_(0);` and `_(NULL);` can be confusing a bit. For instance, clang-18 reports "conversion from 'long' to 'ConstevalStringLiteral' is ambiguous" for the latter.

💬 brunoerg commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2220979817)
Concept ACK