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

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1415130119)
>do we want to issue a warning if only the first outbound connection has a offset larger than our threshold?

This is the main question I guess.
So, with the current code, having 4 peers would never trigger it, right? Because median will always be 0.

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1415132157)
i won't mind that either, but ideally with an in-function comment.

💬 naumenkogs commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1415169393)
yeah that would be better i think

💬 naumenkogs commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1840311256)
ACK 60a487dd2430145115fcd0215472a5a2ef9bb090.

[This](https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1413149420) would be an improvement.

💬 naumenkogs commented on pull request "fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid":
(https://github.com/bitcoin/bitcoin/pull/28997#issuecomment-1840316344)
ACK 38816ff64ed90a55e4879e9b440cdc876302f750

👍 stickies-v approved a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1764382859)
ACK 8a02ce59ffa16c73611aeda6caf8b54d85c6208f

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1415150781)
You're right, I didn't think about the `operator bool`. Thanks.