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

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1415152306)
> But I think just making the call and logging an error when the call fails is the most straightforward approach.

Agreed, can be marked as resolved.

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1415149823)
That makes sense, and I agree with your rationale, so I think my suggestion here in `test/util/index.cpp` indeed should be disregarded. In `node::AbortNode()` though, we are sending an interrupt signal to shutdown the node, so I still think it would make sense [there](https://github.com/bitcoin/bitcoin/pull/28051/files#diff-01b4a8b64504f4f4879e9a7fbac613d882a54975c9629c814e844463fb786a8fR19-R25)? (non-blocking nit, ofc)

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1415185397)
> Lower level code should just be returning errors and notifications, and high level code should be using that information to decide whether to shut down

That does seem like a better design, indeed. I agree then that having the bad approach be more verbose is not something to worry about, and my suggestion should be discarded. Thank you for providing the rationale, I think this might be useful to add to the PR description?