🤔 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?

👍 dergoegge approved a pull request: "fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid"
(https://github.com/bitcoin/bitcoin/pull/28997#pullrequestreview-1764713862)
ACK 38816ff64ed90a55e4879e9b440cdc876302f750

💬 fanquake commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1840511168)
Added the upstream patch to fix Boost Process.

🚀 fanquake merged a pull request: "fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid"
(https://github.com/bitcoin/bitcoin/pull/28997)

💬 hebasto commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1840534127)
@luke-jr
> Weak Concept NACK: We should probably still build the SSE4.1 code if the compiler supports it.

This PR branch still builds the SSE4.1 code if the compiler supports it, no?