💬 kristapsk commented on pull request "log: Don't use scientific notation in log messages":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1923436808)
> > > lgtm ACK [c819a83](https://github.com/bitcoin/bitcoin/commit/c819a83b4d83413a31323c1304664ee4c228ca29). can you update the PR description?
> >
> >
> > Updated.
>
> Are you sure the 'before" is accurate?

Yes, `%g` results in using `%e` or `%f`, depending on what results in shorter string.

👍 theStack approved a pull request: "test: make v2transport arg in addconnection mandatory and few cleanups"
(https://github.com/bitcoin/bitcoin/pull/29356#pullrequestreview-1858800304)
Code-review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235

> > Another possible alternative for addconnection's third parameter handling would be to take as default whatever is set by -v2transport, like we already do it for addnode (i.e. bool use_v2transport = self.MaybeArg(2).value_or(node_v2transport);).
>
> @theStack, i went with the mandatory third parameter approach because:
>
> 1. this can be used only for interaction between python dummy peer(`P2PInterface`) and bitcoind(`TestNode`
...

👍 dergoegge approved a pull request: "refactor: Fix timedata includes"
(https://github.com/bitcoin/bitcoin/pull/29361#pullrequestreview-1858811371)
utACK fad0fafd5aca699cfab7673f8eb18211139aeb18

👍 delta1 approved a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1858823743)
tested ACK 3904123da954f9ebd905de4129aec9f9bab9efc7

confirmed that the test assertion fails without the change to wallet.cpp

💬 kristapsk commented on pull request "log: Don't use scientific notation in log messages":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1923477085)
> > > > lgtm ACK [c819a83](https://github.com/bitcoin/bitcoin/commit/c819a83b4d83413a31323c1304664ee4c228ca29). can you update the PR description?
> > >
> > >
> > > Updated.
> >
> >
> > Are you sure the 'before" is accurate?
>
> Yes, `%g` results in using `%e` or `%f`, depending on what results in shorter string.

Oh, wait, no, accidentally removed precision for "before" when editing, double checked debug.log, fixed.

💬 delta1 commented on pull request "doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex()":
(https://github.com/bitcoin/bitcoin/pull/29355#issuecomment-1923498222)
concept ACK fa027e08f7be63c201f42d0e06160d2273b4a6dd

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1475854412)
` << m_sig` works fine, so `m_sig` can live happily as a `std::array<unsigned char`

📝 maflcko opened a pull request: "refactor: Allow CScript construction from any std::input_iterator"
(https://github.com/bitcoin/bitcoin/pull/29369)
Currently only (pre)vector iterators and raw pointers are accepted. However, this makes it harder to construct from input iterators provided by other classes, such as `std::span`.

Fix that.

💬 glozow commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1923524891)
> i don’t understand why removing the carve out has to be technically linked with v3 deployment.
> you can break carve-out today by broadcasting one of the 2 latest valid commitment tx in lightning full node mempool.
and the other state on the tx-relay network. end of the game. so it can be removed in my mind without talking about v3.

It looks like removal of CPFP carve out is not a concern for you; you just don't think v3 is a good idea. If that's the case, there is no need to post general
...

💬 josibake commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1475841970)
in "wallet: Use scriptPubKey cache in IsMine" (https://github.com/bitcoin/bitcoin/pull/26008/commits/3279e0e8078b92bc590d1ab65ba35f8d197edc12):

Why are we clearing the cache? Wouldn't this invalidate the cache for any existing descriptors I have? If I'm not supposed to do wallet migration in a wallet that already has descriptors, maybe it would be better to check that the cache is empty and throw an error here if not?

💬 josibake commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1475886297)
in "wallet: Use scriptPubKey cache in GetScriptPubKeyMans" (https://github.com/bitcoin/bitcoin/pull/26008/commits/a78ad3f40678d553cbe6a28ed9daaddd3097d196):

nit:
```suggestion
// Search the cache for descriptor scriptPubKeyMans

💬 josibake commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1475828475)
in "wallet: Introduce a callback called after TopUp completes" (https://github.com/bitcoin/bitcoin/pull/26008/commits/eddf9499eff037a94cf28869de6ef9954f64c361):

Seems a bit weird to pass the `topup_callback` when in the same commit we comment that the `LegacyScriptPubKey` can't use the SPK cache due to us not knowing what scripts its watching for.

What about removing it from the `LegacyScriptPubKeyMan` and passing a `nullptr` to `ScriptPubKeyMan` for the call back function, and we can move
...

💬 josibake commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1475871699)
in "wallet: Use scriptPubKey cache in IsMine" (https://github.com/bitcoin/bitcoin/pull/26008/commits/3279e0e8078b92bc590d1ab65ba35f8d197edc12):

While this assumption is true today (and probably always will be), it feels like we are having CWallet make a decision that should be left up to the SPKMan. What if we searched the cache the same way as before and returned the max result from the SPKMan(s)?

```suggestion
isminetype result = ISMINE_NO;
for (const auto& spk : it->se
...

✅ willcl-ark closed an issue: "bitcoin core v.26 shuts down without warning - Doesnt save blocks downloaded"
(https://github.com/bitcoin/bitcoin/issues/29348)

💬 willcl-ark commented on issue "bitcoin core v.26 shuts down without warning - Doesnt save blocks downloaded":
(https://github.com/bitcoin/bitcoin/issues/29348#issuecomment-1923557734)
Hey @Rucade, I'm going to close this now as I don't think there's anything to be done on the Bitcoin Core side.

Feel free to open a new issue if this re-occurs fo you using v26.0

💬 maflcko commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1923585233)
Another test idea would be to check what happens when downgrading to a previous release of Bitcoin Core during the background sync.

💬 fanquake commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475960179)
I think this comment could just be something like `Our usage of mingw-w64 and the msvcrt runtime does not support the x modifier for the _wfopen()`. The other info seems a bit overkiil, I'm also not sure if it's correct. From a quick look, if you choose the runtime you want, when you compile mingw-w64, there's no need for any GCC flag.

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-1923648187)
Alright, code is back in working state. I dropped a bunch of spurious `Make*ByteSpan` (mostly by switching remaining uses of `uint8_t` to `std::byte`).

Also switched to the new logging convention, mostly `LogTrace()`. Also expanded the fuzzer to mess with bytes during the handshake.

💬 fanquake commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1923649409)
> FWIW, the [_wfopen_s](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-s-wfopen-s?view=msvc-170) function reports an error for the x modifier even in Wine.

So it reports an error but the unit tests don't fail? Is this another (different bug)?

💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1923653115)
> > FWIW, the [_wfopen_s](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-s-wfopen-s?view=msvc-170) function reports an error for the x modifier even in Wine.
>
> So it reports an error but the unit tests don't fail? Is this another (different bug)?

We use the `_wfopen`. Microsoft docs [suggests](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=msvc-170):
> More-secure versions of these functions that perform more parameter valida
...