💬 l0rinc commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192276552)
nit: the line is short enough now to be joined
```suggestion
# These environment variables are set by the build process and read by test/*/test_runner.py
```

💬 l0rinc commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192297224)
nit: we might as well update the copyright headers

💬 l0rinc commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192355924)
super-duper-nit (to minimize diff and since this version is found 5x more often in the codebase):
```suggestion
if __name__ == '__main__':
```

💬 l0rinc commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192384775)
we would indeed fail here if we had any errors above 👍

💬 l0rinc commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192441879)
I don't like the hacky path insert, googling a bit indicates we could just exec the whole module, something like:
```python
spec = spec_from_file_location('rpcauth', self.config['environment']['RPCAUTH'])
spec.loader.exec_module(TestRPCAuth.rpcauth := module_from_spec(spec))
```

💬 l0rinc commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192390314)
nit: this seems to duplicate the exact implementation of `password_to_hmac` - not sure how useful that is.

But if we keep it, we could simplify the above to:
```python
from hashlib import sha256
m = hmac.new(salt.encode(), password.encode(), sha256)
```
(since we just checked above that salt and password are always ASCII)

💬 l0rinc commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192595907)
it seems that all other functional tests that have such unit tests are all in `test/functional/test_framework` instead.

<details>
<summary>Details</summary>

```bash
test % grep -r 'unittest.TestCase' .
./functional/feature_framework_unit_tests.py:# the output of `git grep unittest.TestCase ./test/functional/test_framework`
./functional/test_framework/address.py:class TestFrameworkScript(unittest.TestCase):
./functional/test_framework/crypto/ellswift.py:class TestFrameworkEllSwift(uni
...

💬 maflcko commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192666366)

Your new code still looks wrong, when someone sets `HOST=x86_64-apple-darwin` on an arm machine, where signing isn't needed for that case.

Why not use `args.host` and keep the `"arm64-apple-darwin"` string?

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-3049168255)
Looking good mostly. One way I think the review can be made easier is by reordering the commits to make the `isminetype` enum changes appear together because the review is done in the order of the commits presented. Suggested order (builds fine at the second commit on my system):

- [interfaces, gui: Remove is_mine output parameter from getAddress](https://github.com/bitcoin/bitcoin/pull/32523/commits/db7729d368203ac1d622d4f84bc87c818ebaa7a0)
- [wallet: Remove COutput::spendable and Available
...

💬 Sjors commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192695167)
I don't understand the use case. Afaik you can't run the functional tests on such a cross-compiled build?

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3049207085)
> The test isn't testing the indexes code — just its own test-only implementation of it. It was essentially testing itself. And that's not useful.
I don't think so, it does test a case that the current tests don't do: reorg happens in sync period. look at the last part of the test, it tests if the old fork is not in the filter and the new best chain blocks is in the filter, though very basic for now.
The Sync in the test is same as in bitcoind, expect the thread synchronization code which is t
...

👍 brunoerg approved a pull request: "rest: replace `rf_names[0].rf` by `RESTResponseFormat::UNDEF` for code clarity"
(https://github.com/bitcoin/bitcoin/pull/32884#pullrequestreview-2997939984)
code review ACK 6d19815cd44031ff2b45fc9532f579cd81c62749

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3049237953)
> > I don't think the test is useful.
> > Other than that, concept ACK.
>
> You can ignore it.

Easy, everyone. Without specific reasoning, these kinds of claims don’t really help.

💬 ryanofsky commented on pull request "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt":
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3049244656)
Concept ACK. It shouldn't hurt to add more metadata to help detect when there might be a mix of old data in a newer format and new data in an older format in the same wallet.

Conceptually, I think we could always detect these cases by just scanning the entire wallet for data that needs to be upgraded on loading. But having this extra metadata is nice because it can avoid slowing down wallet loading by avoiding unnecessary upgrade rescans.

On the approach in e94ef51837daa5ec9552eb14e8ccc38d
...

💬 maflcko commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192745602)
yeah, right. Probably best to drop the unittest import here to make review easier. Did that, but a bit different from your suggestion.

💬 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.