💬 fjahr commented on pull request "contrib: fix BUILDDIR in gen-bitcoin-conf script and gen-manpages.py":
(https://github.com/bitcoin/bitcoin/pull/31742#issuecomment-2672229030)
Code review ACK 63a8791e15c3ffb44b84ab3e85db62d7997d25fd

✅ fanquake closed a pull request: "Add OP_INTERNALKEY for Tapscript"
(https://github.com/bitcoin/bitcoin/pull/29269)

💬 fanquake commented on pull request "Add OP_INTERNALKEY for Tapscript":
(https://github.com/bitcoin/bitcoin/pull/29269#issuecomment-2672243794)
Closing for now, given #29198 was closed.

✅ fanquake closed a pull request: "Implement OP_CHECKSIGFROMSTACK(VERIFY)"
(https://github.com/bitcoin/bitcoin/pull/29270)

💬 fanquake commented on pull request "Implement OP_CHECKSIGFROMSTACK(VERIFY)":
(https://github.com/bitcoin/bitcoin/pull/29270#issuecomment-2672244425)
Closing for now, given #29198 was closed.

💬 l0rinc commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1964095565)
Thanks for the help, I pushed a new version where every input is restricted (used 100 to be slightly different from the RPC one you've mentioned) and <1000, even though it seems that wasn't the actual bottleneck.

💬 fanquake commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2672258652)
@mzumsande do you want to circle back for another look here? Or @ajtowns?

💬 maflcko commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1964097848)
I was referring to the part of the unit test that checks `!ok_too_small`, which is in the fuzz target guarded by the checksum calculated on the `random_string` (given by the fuzz input). (The if-guard is in the line I commented on, and thus my question)

To clarify, it would be good, if:

* Someone checked that the fuzz engine can break the checksum (ideal).
* Someone added fuzz inputs that pass the checksum (acceptable).
* Alternatively, someone modified the fuzz target to be closer to th
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2672264338)
Rebased 251a55f2f0cc3cdfb7fa0015b76772586134cde3 -> c72b2c2883d4c8791267133f326e3f9347d1520b ([kernelApi_25](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_25) -> [kernelApi_26](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_26), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_25..kernelApi_26))

💬 l0rinc commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1964105142)
> Someone added fuzz inputs that pass the checksum (acceptable).

I have pushed https://github.com/bitcoin/bitcoin/pull/31917 which should resolve this (by randomly generating inputs that can be decoded).

> Someone checked that the fuzz engine can break the checksum (ideal).

I will also add a dedicated fuzzing harnesses to the corpus later.

👍 stickies-v approved a pull request: "qt: Update `src/qt/locale/bitcoin_en.xlf` after string freeze"
(https://github.com/bitcoin-core/gui/pull/854#pullrequestreview-2630752052)
ACK 7267ed051820b9227856143bdf767ae94a5be1d8 - I get the same results when building the `translate` target.

💬 maflcko commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1964109002)
Yes, the high bit is always unset, otherwise the check would fail on both tests.

However that does not mean all remaining bits are always the same in the block hash on regtest and "fuzznet". "fuzznet" doesn't care about the remaining bits at all. regtest does check them.

You can copy-paste the c++ code above and check it for yourself locally.

👍 ryanofsky approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2630774856)
Code review ACK 879569cab4e5b400350f3b95d7bee71b49636591 (no changes since last review other than rebase), but [again](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2631914588) I think this PR should be prefixed with [BREAKING] or [INCOMPATIBLE] or [!] or ‼️ or ⚠️ and have a clear message to developers about what it does like "**Warning**: This PR changes build locations of newly built executables like `bitcoind` and `test_bitcoin` from `src/` to `bin/` without deleting previously b
...

🤔 zaidmstrr reviewed a pull request: "cli: return local services in -netinfo"
(https://github.com/bitcoin/bitcoin/pull/31886#pullrequestreview-2630784513)
Code ACK [724546e](https://github.com/bitcoin/bitcoin/pull/31886/commits/724546e28a5778e1c3f5100406ed7237f76aab55)
I tested the changes on Ubuntu 24.04.2, and it's working fine. I also manually reviewed the changed code for any logical errors.

🤔 jonatack reviewed a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2630770642)
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02

💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1964117116)
maybe clearer

```suggestion
"\nLoad wallet from the -walletdir (default: \"wallets\" in the data directory):\n"
```

💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1964127697)
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the directory of the wallet to be loaded, either relative to the \"wallets\" directory or absolute. The \"wallets\" directory is set by the -walletdir option and defaults to the \"wallets\" folder within the data directory."},
```

The relative path seems more user-friendly, as unloading a wallet that was loaded with an absolute path might require something like

`$ ./build/src/bitcoin-cli
...

👍 pablomartin4btc approved a pull request: "qt: Update `src/qt/locale/bitcoin_en.xlf` after string freeze"
(https://github.com/bitcoin-core/gui/pull/854#pullrequestreview-2630789938)
tACK 7267ed051820b9227856143bdf767ae94a5be1d8

no diff

🤔 pablomartin4btc reviewed a pull request: "doc: update translation generation cmake example"
(https://github.com/bitcoin/bitcoin/pull/31731#pullrequestreview-2630813194)
I think this [would require](https://github.com/bitcoin/bitcoin/pull/31731#pullrequestreview-2627689858) a follow-up, we need to build `depends` with multiprocess otherwise the command would [fail](https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1928888001).

We could use `-DWITH_MULTIPROCESS=OFF`, but I do agree with @hebasto [here](https://github.com/bitcoin/bitcoin/pull/31899#issue-2861109641):

_"At present, multiprocess-specific sources do not contain any translatable strings.
...

✅ fanquake closed a pull request: "refactor: Remove redundant definitions"
(https://github.com/bitcoin/bitcoin/pull/29492)