💬 maflcko commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192745867)
heh, I didn't know. But I'll leave this as-is for now, because the logic is just moved. If this was changed, it could make sense to do it repo-wide for all `sys.path.insert` in a separate pull.

💬 maflcko commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192746255)
leaving as-is for now, as this is move-only and the code could be changed independently in another pull, if there is need.

💬 maflcko commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192748149)
`black` will change those back to `"`. However, to keep it move-only I've changed it back to `'`.

💬 maflcko commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192748491)
thx, done

💬 maflcko commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192748729)
I don't think it is. At least my IDE (vim) doesn't join it. Also, the diff is smaller without joining. Going to leave as-is.

🚀 fanquake merged a pull request: "rest: replace `rf_names[0].rf` by `RESTResponseFormat::UNDEF`"
(https://github.com/bitcoin/bitcoin/pull/32884)

💬 maflcko commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192765394)
I don't have an apple, but i had the impression that rosetta will just run the x86_64 executable on the arm64?

💬 Sjors commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192791070)
Let me try if that actually works...

💬 l0rinc commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#issuecomment-3049494057)
ACK fa4d68cf97b6d77dbb559facbc425376e2c51f62

💬 Sjors commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192923546)
I switched to using `args.host`.

With the previous version:

```sh
cd depends
gmake HOST=x86_64-apple-darwin
cd ..
cmake -B build --toolchain /Users/sjors/dev/bitcoin-rosetta/depends/x86_64-apple-darwin/toolchain.cmake
cmake --build build
```

The result is as expected:

```sh
file -b build/bin/bitcoin
Mach-O 64-bit executable x86_64
```

Meanwhile if you just call `test/get_previous_releases.py`, it's going to download arm binaries:

```
test/get_previous_releases.py
fil
...

💬 Sjors commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#issuecomment-3049540718)
I switched to `args.host`, see https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192923546

Also documented that you can set `HOST`.

💬 maflcko commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-3049542385)
You'll have to rebase. Did you see the immediately previous comment? Also, ref https://github.com/bitcoin/bitcoin/pull/32818

💬 sr-gi commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#issuecomment-3049612125)
I think all feedback has been addressed @mzumsande @sipa @furszy @TheCharlatan @maflcko

Please let me know if there's anything else pending or if anything doesn't make sense.

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2192977836)
> That _is_ indeed confusing - I have tried to unify it as much as I could based on whether it's important that the key is stored as bytes or not. Let me know if it's still confusing or if I forgot anything.

I'd say the `Obfuscation` constructor that takes an uint64_t should probably be removed, because it is unused outside of tests. Also, it exposes an implementation detail, which could even be confusing in the context of endianness. It is only used to set a value of `0` (or all-zero bytes).
...

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2193048005)
Though, generally, I also wonder if the rename from xor to obfuscation is really needed or meaningful. I think it is clear that the xor approach isn't trivial to change without writing data-migration/upgrade code. Also, there hopefully isn't a reason to having to ever change it in the future. Also, there are still plenty of places that refer to xor (`MEMPOOL_DUMP_VERSION_NO_XOR_KEY`, ...).

I'd say just leaving the naming as-is will be beneficial, because:

* The diff is smaller and more foc
...

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193052705)
Instead of magic numbers, try using `TRUC_MAX_VSIZE * WITNESS_SCALE_FACTOR`

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193078705)
Also, there is a separate size limit for TRUC children - I don't think that's been implemented yet?

🤔 glozow reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-2998494185)
Nice, concept ACK! Saw that these tests fail on #31936, which is helpful for showing its issues. Ultimately, I think the wallet commits should be introduced before the RPC ones.

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193049601)
We should also check that we can't fund a v2 transaction when everything is v3 unconfirmed

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193065536)
Comment seems inaccurate: it mentions a set but this only allows for 1 transaction. Should also mention (1) that this is used to stop us from creating another unconfirmed child and (2) this is specifically the in mempool-sibling, as there can be multiple siblings but only 1 in mempool (unless there was a reorg).