💬 brunoerg commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1423095044)
perhaps `change_target` could be `const CAmount&`?

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1423098019)
I have a failing unit test for this :facepalm:

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1850853920)
Re https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1850626464

> And if the kernel is being built for a platform which doesn't support sockets at all, src/util/sock.cpp can just be stubbed or not compiled at all and there should be no problem for portability either.

One of the guiding principles for the kernel project so far has been to prohibit future re-couplings of decoupled modules. The [tracking issue](https://github.com/bitcoin/bitcoin/issues/27587) says "Any further coupli
...

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1423116594)
Ah, I see. I think this is a different set of checks that fail, but I'm not sure exactly what the fix is so maybe both of these can be fixed at the same time...

💬 maflcko commented on pull request "wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs":
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-1850885470)
rebased

📝 maflcko opened a pull request: " refactor: Print verbose serialize compiler error messages "
(https://github.com/bitcoin/bitcoin/pull/29056)
Currently, trying to serialize an object that can't be serialized will fail with a short error message. For example, the diff and the error message:

```diff
diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
index d75eb499b4..773f49845b 100644
--- a/src/test/serialize_tests.cpp
+++ b/src/test/serialize_tests.cpp
@@ -62,6 +62,8 @@ public:

BOOST_AUTO_TEST_CASE(sizes)
{
+ int b[4];
+ DataStream{} << b << Span{b};
BOOST_CHECK_EQUAL(sizeof(unsigned c
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1850924750)
I de-duplicated code in `Send()` by having it call `SendBuf()`. Later I'd like to see if we can reuse some code from `Transport`, since there are some similarities with v2 transport.

I dropped `SendTemplate` and overhauled `SendWork`, `ThreadSv2Handler`, the `COINBASE_OUTPUT_DATA_SIZE` handler and `BuildNewWorkSet` to de-duplicate stuff, make it easier to understand and to avoid passing `m_` variables.

Hopefully I didn't break anything, but I did test on my custom CPU mined signet.

📝 theuni opened a pull request: "Replace non-standard CLZ builtins with c++20's bit_width"
(https://github.com/bitcoin/bitcoin/pull/29057)
Split out of #28674

Note that we can't yet drop our configure checks because we pass the results on to minisketch. I've opened a PR for that upstream here: https://github.com/sipa/minisketch/pull/80

@fanquake [suggested](https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422215535) that we simply replace our `CountBits` call-sites with uses of `std::bit_width` directly and just drop the tests and fuzzers. I agree with that, but I wanted to allow our tests/fuzzers to run with this c
...

🤔 ryanofsky reviewed a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1776092295)
Updated ed0ecc00d627fecde03dddee26b6094499df0195 -> eaf915d61d470372e63f41f11d6a873c1936f73f ([`pr/noshut.21`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.21) -> [`pr/noshut.22`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.22), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/noshut.21..pr/noshut.22)) just implementing a suggested change to rename

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423078042)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1415185397

> Thank you for providing the rationale, I think this might be useful to add to the PR description?

Definitely, and the PR description was pretty bad so I rewrote it. This is explicitly mentioned now at the end.

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423078819)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1388253872

> In `node::AbortNode()` though, we are sending an interrupt signal to shutdown the node, so I still think it would make sense [there](https://github.com/bitcoin/bitcoin/pull/28051/files#diff-01b4a8b64504f4f4879e9a7fbac613d882a54975c9629c814e844463fb786a8fR19-R25)? (non-blocking nit, ofc)

Sorry I missed this comment before, and you are right about AbortNode. I renamed interrupt to shutdown there now and in the KernelN
...

💬 kashifs commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1850966201)
Hi @murchandamus,
I believe that I correctly opened a PR against this PR. Please let me know if this is the correct etiquette or if there is anything more that I should do.

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1850970368)
> One of the guiding principles for the kernel project so far has been to prohibit future re-couplings of decoupled modules.

I reread the issue you linked to, but I don't understand what it implies here. I do think you can rely on linker errors to ensure kernel code isn't calling wallet code or GUI code or P2P code or storing state in global variables. But what else do you want beyond that? And how does removing `sock.cpp` from the util library help, for example?

👍 TheCharlatan approved a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1776264127)
Re-ACK eaf915d61d470372e63f41f11d6a873c1936f73f

💬 eragmus commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850977742)
> going to a casino does not oblige the entire planet to have an indelible mark of fact that you went to a casino. Inscriptions do.

The thing is that inscriptions were not simply possible and begun to be used only when Casey released his software. The concept of inserting arbitrary data was already possible, and thus done, throughout bitcoin's history (this means your node already had arbitrary data, including various distasteful data). You can do a simple google search and see research papers
...

💬 sinetek commented on issue "The logo icon doesn't show properly under Wayland":
(https://github.com/bitcoin-core/gui/issues/781#issuecomment-1850989318)
> a while ago, and I notice

it sohuld be simpler than that, something like QWindow::setIcon() ?

👍 furszy approved a pull request: "tests, bench: Fix issue with CWallet::LoadWallet() being called in the wrong places"
(https://github.com/bitcoin/bitcoin/pull/29055#pullrequestreview-1776302056)
ACK bd7f5d33

For a follow-up, we should make `CWallet::LoadWallet()` private and replace all those calls for `CWallet::Create()`.

📝 mzumsande opened a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058)
Some preparations before enabling `-v2transport` as the default:
* Use v2 for `-connect`, `-addnode` config arg and `-seednode` if `-v2transport` is enabled.
Our peer may of may not support v2, but I don't think an extra option is necessary for any of these (we have that for the `addnode` rpc), because we have the reconnection mechanism that will try again with `v1` if our peer doesn't support `v2`.
* Add a column for the transport protocol to `-netinfo`. I added it next to the `net` column
...

📝 mzumsande converted_to_draft a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058)
Some preparations before enabling `-v2transport` as the default:
* Use v2 for `-connect`, `-addnode` config arg and `-seednode` if `-v2transport` is enabled.
Our peer may or may not support v2, but I don't think an extra option is necessary for any of these (we have that for the `addnode` rpc), because we have the reconnection mechanism that will try again with `v1` if our peer doesn't support `v2`.
* Add a column for the transport protocol to `-netinfo`. I added it next to the `net` column
...

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423225639)
> Ah oops my bad. I was backporting my proposed refactor from [5b8c165](https://github.com/bitcoin/bitcoin/commit/5b8c165c2ae0448b802c0c4907303d016f301f0a). I use a feerate of 3000 ṩ/kvB there, but overlooked that it was still set to 0 here. Since the long-term feerate is set to 1000 ṩ/vB here, this makes our transaction building follow high-feerate behavior.
>
> I’ve edited my code suggestion in the above comment to correct the feerate.

the approach is better but still fail. Knapsack retr
...