🤔 hebasto reviewed a pull request: "lint: Check for missing bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29408#pullrequestreview-1885268833)
https://api.cirrus-ci.com/v1/task/5330278671974400/logs/ci.log:

```
crypto/sha256.cpp should remove these lines:
- #include <config/bitcoin-config.h> // lines 6-6
```

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1948481906)
Windows test fails with:

```
test/sv2_noise_tests.cpp(62): fatal error: in "sv2_noise_tests/certificate_test": critical check !alice_certificate.Validate(XOnlyPubKey(alice_authority_key.GetPubKey())) has failed
```

Odd. Part of https://github.com/bitcoin/bitcoin/pull/29346 so can be debugged there.

💬 vasild commented on pull request "refactor: Compile unreachable walletdb code":
(https://github.com/bitcoin/bitcoin/pull/29315#issuecomment-1948486081)
> When unreachable code isn't compiled, compile failures are not detected.

Another approach to this is to run CI with both `USE_BDB` defined and `USE_BDB` undefined.

> Fix this by leaving it unreachable, but compiling it.

This produces [compiler warnings](https://github.com/bitcoin/bitcoin/issues/29334) about unreachable code. I guess that if we are to have deliberately unreachable code, then some pragma is needed to tell compilers not to warn about it in those particular places where i
...

💬 vasild commented on issue "Clang 14 emits `-Wunreachable-code` warnings":
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1948489550)
I see, commented in https://github.com/bitcoin/bitcoin/pull/29315#issuecomment-1948486081

💬 theStack commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-1948492950)
Concept ACK

💬 Kixunil commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1948497448)
> Removal happens at some unknown time after deprecation

That doesn't help people who really needed the library. Yeah, they will get screwed later but still they are getting screwed. I meant period to voice opposition to this change. Unless you're open to un-deprecating of course.

> What exactly do you mean by "verified"?

It must be possible to include it in a block at some later time when the parent is confirmed and time locks are satisfied. Basically the same requirement as LN force c
...

💬 ryanofsky commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492564530)
In commit "wallet: Retrieve ID from loaded DescSPKM directly" (ac246f68299d0dc208ae513dfad1a8fc91b5e6d4)

Not important, but you could avoid the dynamic_cast and remove some boilerplate with:

```c++
DescriptorScriptPubKeyMan& CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc)
{
DescriptorScriptPubKeyMan* spkm;
if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
spkm = new ExternalSignerScriptPubKeyMan(*this, desc, m_keypool_size);
} else {

...

🤔 ryanofsky reviewed a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1885278916)
Code ACK ac246f68299d0dc208ae513dfad1a8fc91b5e6d4

Looks good, but wondering about a possible bug in the GetScriptPubKeyMans legacy case (see below)

💬 ryanofsky commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492548296)
In commit "wallet: Use scriptPubKey cache in GetScriptPubKeyMans" (b9dd0fecda98bd14d8d305a69e6ce65f1e0a73fa)

This also needs to check GetLegacyScriptPubKeyMan()->CanProvide() to match previous behavior, I think

💬 hebasto commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#issuecomment-1948517549)
@glozow

Feel free to pick up the https://github.com/hebasto/bitcoin/commit/fb29011cbec8997c70f30797bbba3784702158a0 commit from the https://github.com/hebasto/bitcoin/commits/240216-26.1rc1-tr/ branch to pull the recent translations from the Transifex website.

(same as https://github.com/bitcoin/bitcoin/pull/28763 etc)

💬 maflcko commented on pull request "refactor: Compile unreachable walletdb code":
(https://github.com/bitcoin/bitcoin/pull/29315#issuecomment-1948526834)
> Another approach to this is to run CI with both `USE_BDB` defined and `USE_BDB` undefined.

This is already done, but generally it is better to notify devs of compile failures immediately, instead of going through the lengthy CI pipeline.



> This produces [compiler warnings](https://github.com/bitcoin/bitcoin/issues/29334) about unreachable code. I guess that if we are to have deliberately unreachable code, then some pragma is needed to tell compilers not to warn about it in those part
...

🤔 kevkevinpal reviewed a pull request: "test/BIP324: disconnection scenarios during v2 handshake"
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-1884162177)
Concept ACK

added two comments I will continue to test these changes as I am still trying to understand fully what the tests do exactly

💬 kevkevinpal commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491848110)
do you think it would make sense to use `test_earlykeyresponse` here and then make `test_v2disconnection` into `run_test`

💬 kevkevinpal commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491897558)
Are the `test_type`'s in `test_v2disconnection` dependent on `test_earlykeyresponse` to run first?

I tried running the tests individually for each `test_type` but I ran into this error, using this diff a74f4ddcc50eeba04977d8eeb96ffee2a94dbbf9
```
2024-02-16T02:38:17.055000Z TestFramework (INFO): PRNG seed is: 8074818075718275852
2024-02-16T02:38:17.056000Z TestFramework (INFO): Initializing test directory /var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_dmwxb4ar
2024-02
...

💬 vasild commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1492622692)
I can reproduce now, my bad probably removing the newlines from the base64 input (even though my `base64` tool succeeds in decoding that).

📝 maflcko opened a pull request: "ci: Avoid CI failures from temp env file reuse"
(https://github.com/bitcoin/bitcoin/pull/29441)
* Currently, running separate CI tasks at the same time may intermittently fail, because they race to read/write `/tmp/env`. Fix this by adding `$CONTAINER_NAME` to the file name.

* Also, add `$USER`, while touching the line, to allow different users to run the same CI task at the same time.

* Also, skip the git install if there is no need.

Ref: https://github.com/bitcoin/bitcoin/pull/29274

💬 hebasto commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#issuecomment-1948586673)
To fix the "macOS 13 native" job failure we should backport https://github.com/bitcoin/bitcoin/pull/29195.

👍 vasild approved a pull request: "rpc: Fixed signed integer overflow for large feerates"
(https://github.com/bitcoin/bitcoin/pull/29434#pullrequestreview-1885414888)
ACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

💬 vasild commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492652437)
What was the goal before? If it was for some load balancing, e.g. if it returns 3 addresses to which we can connect to not always connect to the first result? (or have two different nodes connect to the first result)

If "yes" then that still holds with this PR?

💬 glozow commented on pull request "test: merge banning test from p2p_disconnect_ban to rpc_setban":
(https://github.com/bitcoin/bitcoin/pull/26863#issuecomment-1948661364)
Since there doesn't seem to be very much review interest... is there any benefit in moving these tests to a different file?