🤔 pablomartin4btc reviewed a pull request: "Fee Estimator updates from Validation Interface/CScheduler thread"
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1763789905)
post merge tACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2

Tested manually using `RPC` commands `estimatesmartfee` and `estimaterawfee` (following cases from `/test/functional/feature_fee_estimation.py`).

💬 luke-jr commented on pull request "index: block filters sync, reduce disk read operations by caching last header":
(https://github.com/bitcoin/bitcoin/pull/28955#issuecomment-1839900641)
>Probably, it is because the benchmarks, same as the unit tests, create temp directories. Which may be faster to access than regular directories.

Is it, though? I would think the OS should have this cached regardless?

🤔 luke-jr requested changes to a pull request: "build: disable external-signer for Windows"
(https://github.com/bitcoin/bitcoin/pull/28967#pullrequestreview-1763820931)
Let's at least let builders explicitly enable it?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1839920097)
@martinus thank you very much for your review! I updated the PR with all your suggestions.

💬 luke-jr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1414811800)
There is no seed phrase.

💬 luke-jr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1414812287)
Someone with such access can just install a keylogger.

💬 luke-jr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1414810714)
No need to say "introduced in" especially since v0.4.0 was not Bitcoin Core in any meaningful way

💬 luke-jr commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#issuecomment-1839933366)
utACK

🤔 luke-jr requested changes to a pull request: "Replace Boost.Process with cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1763876066)
>>Maybe a subtree from our own fork?

>I think that would mostly just re-introduce the awkwardness we ended up with with univalue, for not much benefit, and for code that shouldn't be critical (i.e compared to leveldb & libsecp256k1), this is for an optional feature, that is not used by a large proportion of the people running this software.

Which means it should be an entirely external dependency we just check for in configure...

The only thing worse than a subtree, is entirely vendorin
...

💬 jonatack commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414836494)
This is now a bit verbose and redundant, and does not address https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414581640.

Suggest:

"Therefore, it is crucial to securely back up the newly generated wallet file using the `backupwallet` RPC."

💬 martinus commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1840061587)
I didn't think this through yet, but this might be an alternative way to achieve the same thing: Instead of the double linked list, split the `cacheCoins` map up into 4 different maps, one map for each flag setting, e.g. as a `std::array<CCoinMap, 4> m_cache_coins_per_flag;`. Index is the flag, e.g. `m_cache_coins_per_flag[FRESH | DIRTY]` to get the map with all entries that are fresh & dirty. Instead of setting a flag & linking the list, use the unordered_map's `auto node = extract()` and `ins
...

💬 maflcko commented on issue "build: Configuring with `-mno-sse4.1` does not fail the sse4.1 instrinsics check":
(https://github.com/bitcoin/bitcoin/issues/28864#issuecomment-1840181002)
> This is fixed by #13789 which was inexplicably closed for no reason

It was closed due to https://github.com/bitcoin/bitcoin/issues/13758#issuecomment-1216319878

💬 maflcko commented on pull request "crypto/sha256: Use pragmas to enforce necessary intrinsics for GCC and Clang":
(https://github.com/bitcoin/bitcoin/pull/13789#issuecomment-1840184719)
(This was closed as part of commit 02aefa169a9e6ed12c7bd8f3392adcd073d8d56b)

💬 maflcko commented on issue "getrawtransaction xxxxxx.... 2 causes a segfault":
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1840199422)
> when "verbosity" = 2, that allows the code to fall all the way to TxToJSON with a bad pointer in hash_block where it will not do the right thing.

hash_block is not a pointer. If it was unset, but read, it would just be all-zero.



> After restarting the daemon and since then including now, the rpc command correctly tells me that:
> "No such mempool transaction. Use -txindex or provide a block hash to enable blockchain transaction queries. Use gettransaction for wallet transactions."

...

🤔 S3RK reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1756870858)
Code review ACK ac3b860bff4279ff470560d0ccad9b2fc09672a9

I carefully re-reviewed the new approach. Looks good to me. Going to repeat the tests now.

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1413520408)
in 14738e9a12c3d128f52875cf56b2ecacc6e62642

Can we make `key` optional to avoid passing invalid keys?

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1413528877)
in eca964a8c07cf43b45e2c0482614ded390d42fec

nit: Could also `Assume` that decryption is successful

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1413558248)
in 14738e9a12c3d128f52875cf56b2ecacc6e62642

nit: should we make the variables optional to avoid storing invalid keys?

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1410304313)
in 14738e9a12c3d128f52875cf56b2ecacc6e62642

nit. The code is used during wallet loading. Should we instead of `Assert` return error so we can log proper corruption error?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1840243574)
@sr-gi implemented your suggestion and moved the last commit to the front, so that these legacy field don't distract reviewers.