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

💬 maflcko commented on pull request "type-safe(r) GenTxid constructors":
(https://github.com/bitcoin/bitcoin/pull/28658#discussion_r1415395926)
I don't think `explicit GenTxid(...)` requires changing a bunch of code. It is simply a constructor that can be used by new code, and old code can (for now) use the old methods to create the object.

Should be fine to add and use it, maybe as part of one of the follow-ups to convert more code?

💬 maflcko commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1415458663)
One thing I don't like about `modernize-use-equals-default` is that it makes aggregate initialization possible again, when a developer disabled it by declaring a constructor.

Though, now that C++20 will be required soon, I guess it can be enabled. C.f.

```cpp
struct A {
// A() {}; // "main" fn fails to compile under C++17 and 20
A() = default; // "main" fn fails to compile under C++20
int b{};
};

int main() { (void)A{1}; }

💬 maflcko commented on pull request "fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid":
(https://github.com/bitcoin/bitcoin/pull/28997#issuecomment-1840649876)
> Fixes the bugs in the fuzz test with no more changes as an alternative to #28658

Can you explain the "bugs"? The wtxid is equal to the txid for transactions that have not witness data, and all transactions in this test have no witness data.

Seems fine to fix the style and make this refactor, but I don't see any bugs.

Also, if there was a bug, the test should be fixed to catch it, no?