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

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

Currently, there are stuff that are being test in both `p2p_disconnect_ban` and `rpc_setban` (e.g. "Test the ban list is preserved through restart" in `setban` and "setban: test persistence across node restart" in `p2p_disconnect_ban`). Since we have a specific test for `setban` RPC, the idea is concentrate everything about it there.

🤔 tdb3 reviewed a pull request: "rpc: Fixed signed integer overflow for large feerates"
(https://github.com/bitcoin/bitcoin/pull/29434#pullrequestreview-1885475248)
Good work on the find and fix.
Seems to work well.
Code review ACK and basic test ACK for dddd7be9bf038c25f3e53c5bd708fb9cf73d4493.


Test actions taken:
Checked out PR branch, built, ran all unit and functional tests (all passed). As a basic sanity check, created a regtest transaction with high fee rate (> 1 BTC/kvB) and executed testmempoolaccept/sendrawtransaction on both the PR branch and v26.0. v26.0 allows and PR code rejects with expected error -8.


v26.0:
```
$ bitcoin-c
...

💬 maflcko commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1948718171)
lgtm ACK dbdb7320252fd68415e8b76bad5a2169bd7da241

Seems fine to log when starting to dump the mempool.

👍 Sjors approved a pull request: "ci: Avoid CI failures from temp env file reuse"
(https://github.com/bitcoin/bitcoin/pull/29441#pullrequestreview-1885505066)
ACK

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1948788558)
Thank you for the comments,

Updated 316c7c845036cbffa22b9e44f31dca8573ffb639 -> d5228efb5391b31a9a0673019e43e7fa2cd4ac07 ([noGlobalSignals_18](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_18) -> [noGlobalSignals_19](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_19), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_18..noGlobalSignals_19))

* Addressed [comment_1](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492523347), [c
...