💬 maflcko commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1475695984)
> I'll look into replace `write` with `>>`.

`>>` means reading. You may want to try `<<`, if you want to write an object.

💬 maflcko commented on pull request "assumeutxo, rpc: Add 'start' parameter to loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1923278780)
I think it is fine to not have an abort command for now, because currently it is also not possible to abort, unless the complete process is shut down. But up to you.

💬 maflcko commented on pull request "log: Don't use scientific notation in log messages":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1923279799)
lgtm

⚠️ ttiiggss opened an issue: "(website) Piotr Piasecki link to Testnet Faucet gives a errror 500."
(https://github.com/bitcoin/bitcoin/issues/29368)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Tried to claim some testnet coins, used the link [here](https://developer.bitcoin.org/examples/testing.html)

Got a error 500. Tried to contact Piotr Piasecki but cannot find details.

### Expected behaviour

Testnet Faucet link to resolve.

### Steps to reproduce

Click [This](https://tpfaucet.appspot.com/)

### Relevant log output

_No response_

### How did you obtain Bitcoin Core


...

✅ fanquake closed an issue: "(website) Piotr Piasecki link to Testnet Faucet gives a errror 500."
(https://github.com/bitcoin/bitcoin/issues/29368)

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

Are you sure the 'before" is accurate?

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1923423102)
Squashed down the changes, and did some Guix cleanup.
We can entirely remove the GCC toolchain from the macOS build now. Apart from removing unused deps,
my other reason to do this was to make sure that `ld` wasn't even in the build environment, so couldn't be getting used over `lld`.

However this has come with a new issue. Qt now fails to build, because it's trying to build qmake with `g++`,
which no-longer exists. This would seem to be a bug in any case, because if we are using a clang
...

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