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

💬 glozow commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#issuecomment-1948789500)
Thanks @hebasto, I've added your commit

💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492705747)
You can have optional stuff with `shared_ptr` as well (if it is empty).

👍 ryanofsky approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1885543942)
Code review ACK d5228efb5391b31a9a0673019e43e7fa2cd4ac07. Just comment change since last review.

👍 hebasto approved a pull request: "[26.1] final changes for 26.1rc1"
(https://github.com/bitcoin/bitcoin/pull/29440#pullrequestreview-1885558510)
ACK 3d789657a8bf97089051348d472d7143a37ee988.

💬 hebasto commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#discussion_r1492723273)
nit: Why not the same `help2man` version?

💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492732536)
> this seems like an argument to always use shared_ptr or unique_ptr everywhere and never use plain pointers or references.

Raw pointers and references are ok to be used for arguments to functions. Or as automatic variables that obviously don't outlive the referenced object (on-stack local variables inside the body of a function).

> If you would not want a plain reference to be stored here, is there any case where you would want one to be stored?

If you store raw pointers or references
...

📝 remyers opened a pull request: "wallet: Target a pre-defined utxo set composition by adjusting change outputs"
(https://github.com/bitcoin/bitcoin/pull/29442)
This PR is designed for the use case of a Lightning node that provides liquidity of predefined amounts via [liquidity ads](https://github.com/lightning/bolts/pull/878).

Coin selection is currently optimized to reduce the size of the utxo set and create change optimized for privacy. A liquidity provider instead needs to service multiple liquidity requests by spending confirmed utxos of known sizes.

Ideally most liquidity transactions would be funded by a single input or a small set of input
...

💬 glozow commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#discussion_r1492742188)
I suppose that's because fanquake and I use different machines

👍 vasild approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1885604077)
ACK d5228efb5391b31a9a0673019e43e7fa2cd4ac07

I would be happy to re-review if this is changed to use automatic memory management everywhere (https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489457095 and https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489527335). Or at least add a comment next to the raw pointers/references like "this points to NodeContext::validation_signals and should not be used after that is destroyed".

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1492766771)
this is odd, should we just `Assume()` `replacement_vsize>0` in `CalculateFeerateDiagramsForRBF`?

💬 vasild commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1492773923)
Yes! See the first two commits in https://github.com/bitcoin/bitcoin/pull/26812:

bee6bdf0a5 `test: put the generic parts from StaticContentsSock into a separate class`
f42e4f3b3b `test: add a mocked Sock that allows inspecting what has been Send() to it`

and then how to use that in the last commit of the same PR:

8b10990aa0 `test: add unit tests exercising full call chain of CConnman and PeerManager`

With those it is possible to send/receive raw bytes to/from the (mocked) socket, or
...