👍 willcl-ark approved a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302)
ACK ac9fee615

@ryanofsky Thanks for the extra clarification. I agree (and have now tested) that this PR does achieve that correctly.

I left a few tiny nits that don't need to be adressed unless you retouch (and want to address them).

💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150313881)
On a re-touch similar re-wording could be done here?

💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150313450)
If you re-touch, perhaps this could read:

'Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored '

💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150550636)
There is `get_datadir_path()` in _test_framework/util.py_:

https://github.com/bitcoin/bitcoin/blob/b759cefe936ed3991633acff215ea1dcec5ece28/test/functional/test_framework/util.py#L417-L418

So perhaps could go next to that in a future PR?

💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150571443)
^ this can be resolved.

💬 TheCharlatan commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1486858153)
Should this get its own release note? There are bound to be some node operators that will be caught off-guard by this change.

💬 ismaelsadeeq commented on pull request "test: use address_to_scriptpubkey instead of RPC call":
(https://github.com/bitcoin/bitcoin/pull/27349#issuecomment-1486864385)
> @ismaelsadeeq seems ready for review to me
Ready for review now.

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150513279)
nit: could use structured bindings.
```c++
for (const auto& [dest, entry] : m_address_book) {
for (const auto& [id, request] : entry.receive_requests) {
values.emplace_back(request);
}
}
```

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150541159)
Would be good to keep the encapsulation and don't access `m_address_book` from outside of the wallet class (otherwise we are reverting #25337 and, future, #26836).

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150538791)
This method was removed from the cpp but not from the header.

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150523650)
No longer the case.
Same for the comment that is above this one.

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150567795)
this also fixes abdef47f206dec114300b2d852f5c297b03179e3 (from #26644)

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150535358)
Not really for this PR, but.. the `used` arg is never false.

`SetAddressUsed` is only called from `SetSpentKeyState` which is only called from `AddToWallet`, which provides `used=true`.

Which.. means that if the address is not reverted to a "not used" state if the tx gets abandoned/discarded.

Could also work on it on a follow-up. Probably after #26836 so changes don't end up conflicting that much.

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150572914)
Could
```c++
BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::::LOAD_OK);
```

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150579720)
Same as above, would be good to not access `m_address_book` directly from outside of the wallet class.
The map [] operator usage on a random place of the sources can easily screw things up.

Could change it to
```c++
BOOST_CHECK(!wallet->IsAddressUsed(PKHash()));
```

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150583313)
```c++
BOOST_CHECK(wallet->IsAddressUsed(PKHash()));
BOOST_CHECK(wallet->IsAddressUsed(ScriptHash()));
```

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150586523)
```c++
BOOST_CHECK(!wallet->IsAddressUsed(PKHash()));
BOOST_CHECK(!wallet->IsAddressUsed(ScriptHash()));
```

👍 hebasto approved a pull request: "guix: use GCC tool wrappers"
(https://github.com/bitcoin/bitcoin/pull/27345)
ACK 4133c8104f522c403c55d26bd03436a8149ff106

Guix builds:
```
df90584fe5c1c68ec6a547ba04b81b66fae61035d5d4b586bc2c70f19176bc62 guix-build-4133c8104f52/output/aarch64-linux-gnu/SHA256SUMS.part
ba5553f64f73d84850ffe4e2c1712bd803c6e6f13b9f57cbc82eecab8e9e82c1 guix-build-4133c8104f52/output/aarch64-linux-gnu/bitcoin-4133c8104f52-aarch64-linux-gnu-debug.tar.gz
316e07c7baae2ff35e929c25d472ef0b66077b275c2c55f4b24acdd015947e80 guix-build-4133c8104f52/output/aarch64-linux-gnu/bitcoin-4133c8104f
...

🚀 fanquake merged a pull request: "ci: Use TSan new runtime (llvm-16, take 3)"
(https://github.com/bitcoin/bitcoin/pull/27298)

💬 ismaelsadeeq commented on pull request "test: use address_to_scriptpubkey instead of RPC call":
(https://github.com/bitcoin/bitcoin/pull/27349#issuecomment-1486878414)
> ACK [c73a065](https://github.com/bitcoin/bitcoin/commit/c73a0658c2c5c43d4ba2d8dbf2af71abe84b40e8)
>
> Nice to see a standard function for getting the `scriptPubKey` instead of a mix of RPC calls. Removing the RPC call _should_ make the tests run faster, but considering it wasn't used in many places, I doubt it makes a material difference. Left two non-blocking nits.
>
> I also noticed the CI was failing, but when I rebased on master and ran the failing functional test locally, everything
...