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

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475964652)
> 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.

If the new runtime is available in guix, I'd say it would be better to just use it, instead of patching the code. Or is there some downside I am missing?

💬 fanquake commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475966162)
> If the new runtime is available in guix, I'd say it would be better to just use it, instead of patching the code. Or is there some downside I am missing?

I agree that we should move to the new runtime. I assume the downside is possibly dropping support for Windows 7, but I don't see why that would be a problem.

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475966908)
> > 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.
>
> If the new runtime can be linked in guix builds, I'd say it would be better to use it, instead of patching the code. Or is there some downside I am missing?

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475969263)
According to the docs, if it is missing, it can be installed via "Windows Update": https://devblogs.microsoft.com/cppblog/introducing-the-universal-crt/ . IIUC it is shipped by default on Windows 10.

👍 stickies-v approved a pull request: "refactor: Fix timedata includes"
(https://github.com/bitcoin/bitcoin/pull/29361#pullrequestreview-1859084735)
ACK fad0fafd5aca699cfab7673f8eb18211139aeb18