💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1927304095)
> > It also seems strange to me that all of this care is being taken to keep the secret data secure in memory when you are reading writing it unencrypted to and from disk.
>
> That's literally what the wallet does by default. `CKey` provides extra security for free, so it's better to use it. The optional wallet encryption feature makes use of this security feature, by ensuring that once a wallet is (re)locked the key data is erased from memory.

https://github.com/bitcoin/bitcoin/blob/maste
...

💬 aureleoules commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1927311589)
@theuni yeah the `AddrManGetAddr` and `PrevectorDeserializeNontrivial` are known to be flaky. I'll filter them out from the UI while I find a better solution.
Some benchmarks depend on I/O or randomness so it's hard a have consistent results between runs.

You can see ignored benchmarks here in the meantime: https://github.com/corecheck/frontend/blob/master/src/routes/%5Bowner%5D/%5Brepo%5D/pulls/%5Bnumber%5D/Benchmarks.svelte#L334

👍 dergoegge approved a pull request: "fuzz: Generate with random libFuzzer settings"
(https://github.com/bitcoin/bitcoin/pull/28178#pullrequestreview-1863171718)
utACK fa3a4102ef0ae06d8930d7a7b567759e2a5b5fde

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1927349134)
Added to `2024/02/sv2-poll-ellswift`: we now hold on to templates for 10 seconds after a new block arrives, in a case a miner was still working on it. The resulting block still wouldn't get broadcast.

💬 aureleoules commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1927351585)
Rebased. I had to change the offset again in `feature_assumeutxo.py`.

💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1927359849)
@aureleoules Thanks, that's helpful.

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1927368536)
> In your case, loss of a passphrase would simply mean regenerating a new key.

There is no passphrase for sv2 keys. It would make running a template provider much too complicated. It could be added an advanced feature later of course.

> 'd be more inclined if this PR was proposing a general refactor

I just need to store a key a disk, I'm not interested in a general refactor.

> You're not blocked on this PR being merged, and it seems we wont need this change if the other PRs don't pro
...

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1927418035)
Last push made all the debug strings include txid and wtxid of transactions - @sdaftuar pointed out it's more useful to have both.

💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-1927420896)
How/when would the file be closed?

👍 hernanmarino approved a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1863286073)
re ack f822ac9a24d684937f1258da89812e99c4b205ba

👍 brunoerg approved a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-1863342508)
utACK 5e3c80da1473f90406b84c1ba14dc563ce4d2853

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478561706)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998490

> Any reason to change the sequence id? I think this isn't changed in `loadtxoutset` either?

I'll double check and add a comment. I think this might be needed to satisfy an assert that ensures nChainTx is set if nSequenceId is set, since these are both set when a block is received and all blocks before it have also been received.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478549351)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477981888

> Any reason to touch this LOC?

No, will restore. I only meant to update the comments around it.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478567668)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477996772

> isn't this always true? Maybe you meant to type `-` instead of `<`?
>
> In any case, the test seems to be passing either way, so this is "dead" code, similar to a code comment.

I think it was supposed to be i < last_assumed_valid_idx - 1, to preserve nChainTx in the snapshot block, and only set nChainTx to 0 before the snapshot block, so the test matches what happens in reality. But it does seem like this is not
...

🤔 ryanofsky reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1863282117)
Thanks for the close review! Will work on your suggestions

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478584012)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478006900

> Is removing the validity check intended?

It was intended, but on second thought it is probably better to add back. The reason for removing it is that the block reaching VALID_TRANSACTIONS level is not really significant, because what is actually needed is for the block and all its ancestors back to the snapshot block or genesis block to reach VALID_TRANACTIONS level, and HaveNumChainTxs() is the direct and efficient
...

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478594492)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478022794

> Any reason to remove this check?

No, good catch, I'll add it back. I was thinking it wasn't really related to the other checks here, and trying to eliminate unnecessary uses of BLOCK_ASSUMED_VALID / IsAssumedValid in general. But as long as we have the assumed valid flag, might as well check this.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478604893)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478018977

> Can this be further limited to only mark the snap_block if it has ASSUMED_VALID set? Otherwise, nodes which never used loadtxoutset will assume that some block is a "snapshot block", even though they never used that feature.

I'm pretty sure I can do that, but it should not change anything. If loadtxoutset is not used, GetSnapshotBaseBlock will return null and snap_block should already be false. The only case where t
...

📝 brunoerg converted_to_draft a pull request: "wallet, rpc: add BIP44 `account` in `createwallet`"
(https://github.com/bitcoin/bitcoin/pull/29129)
This PR adds an `account` parameter in `createwallet` RPC. It's the
BIP44 account that will be used in `SetupDescriptorScriptPubKeyMans`
to fetch the descriptors from the external signer.

⚠️ dpc opened an issue: "Default rpcthreads value (40 is way too small"
(https://github.com/bitcoin/bitcoin/issues/29386)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I keep having to increase `rpcthreads` about everywhere I'm using bitcoind because without it it's not very robust/performant.

Pasting verbatim my comment in [bdk that just hit the same issue](https://github.com/bitcoindevkit/bdk/pull/1320#issuecomment-1926231768):


> From a perspective of most workloads e.g. asking for blockchain data, `bitcoind` (and `electrs`) acts very much like
...