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

Maybe give another look at the test code before saying that? https://github.com/bitcoin/bitcoin/commit/21ade546808fd757bf451db74cea1da3755f6613

The test isn't testing the indexes code — just its own test-only implementation of it. It was essentially testing itself.

🤔 furszy reviewed a pull request: "wallet: remove dead code in legacy wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32758#pullrequestreview-2997688329)
ACK 150b5c99ca11885ef15d04139d919d734e2c211a

⚠️ fanquake opened an issue: "guix: Windows build is non-deterministic across build architectures"
(https://github.com/bitcoin/bitcoin/issues/32923)
This has been the case since https://github.com/bitcoin/bitcoin/commit/8578fabb95face25d9fa7dfaad38d0a2857c2481.

Guix build on `x86_64` & `aarch64` @ 01f908195589 (prior merge):
```bash
0324606fdfc525e2acf6ffc5b5327021df6b47de107dbeb51f54162a3dead570 guix-build-01f908195589/output/dist-archive/bitcoin-01f908195589.tar.gz
e9be0c0c6ab0d856c2176ff0da9c4fb3599f8bd0d578138262fde7307691c28b guix-build-01f908195589/output/x86_64-w64-mingw32/SHA256SUMS.part
b27afe43704fa3045f1acebd967f282781f5d1907b4
...

💬 fanquake commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3049041023)
The non-determinism issue is not being introduced with this change, it's already present in master, see #32923. Given that, It'd be good to move ahead here, so we can finished up the backports for `29.1`.

💬 fanquake commented on pull request "wallet: Always set descriptor cache upgraded flag for new wallets":
(https://github.com/bitcoin/bitcoin/pull/32597#issuecomment-3049044422)
As far as I can tell, this change is the cause of non-determinism when Guix building the Windows binaries, on different architectures: #32923.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3049054361)
> Can you recap what the deficiency was?

Duplicate transactions do not count towards memory usage. When we exceed memory limits, the worst case happens when we need to delete a huge amount of announcements before finding a unique one. So `OrphanageMultiPeerEviction` needs to delete all but the last 2 transactions for each peer before it makes any progress on the memory usage.

For 24k, I was getting 10s of milliseconds for `OrphanageMultiPeerEviction` and even worse for `EraseForBlock` (I c
...

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3049095473)
Rebased

👍 l0rinc approved a pull request: "test: Turn rpcauth.py test into functional test"
(https://github.com/bitcoin/bitcoin/pull/32881#pullrequestreview-2997269889)
ACK fa8a55e116ba925ce1791605a3ac55bc005f88bf

This is a continuation of the migration efforts started in #32697.
It makes sense to unify this with the rest of the tool tests - though it differs from the others in structure, please see my suggestion for how we could make it more like the other functional tool tests.

💬 l0rinc commented on pull request "test: Turn rpcauth.py test into functional test":
(https://github.com/bitcoin/bitcoin/pull/32881#discussion_r2192293951)
note: was wondering who else is using `RPCAUTH=@abs_top_srcdir@/share/rpcauth/rpcauth.py` below - ran all functional tests and `rpc_users.py` (and `tool_rpcauth.py` of course) are still using it.

This also demonstrates why the current change makes sense - and that the same env is read successfully and that the test is indeed run automatically now as part of the functional test suite 👍!

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