💬 sdaftuar commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424084885)
nit: I found the "not" in this comment confusing.

💬 sdaftuar commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424077858)
Is there a reason to drop the `txns.size() > 1` test in this `if` clause?

💬 sdaftuar commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424392025)
I was trying to figure out exactly why this code is safe in a package RBF context. I think for a generalized package RBF, this would not be safe, because we do not verify that a child transaction in an incoming package (say) isn't spending a coin in the mempool that would be conflicted by some other transaction in the package.

However, in this particular case, we are enforcing in `PackageMempoolChecks()` (further down) that the incoming package has **no** other in-mempool ancestors (ie the ne
...

🤔 ryanofsky reviewed a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1778060101)
Updated 6d8cb4d4cd6062ad06cd351439e9139e028995d3 -> 9ba4dc41fd405a008e9badc4092ef1905f534b96 ([`pr/rmutil.6`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.6) -> [`pr/rmutil.7`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/rmutil.6..pr/rmutil.7)) fixing crypto descriptions in libraries.md

re: https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1776320307

>* moving the spanparsing stuff to script mo
...

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1424337096)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423218395

> Doesn't libconsensus necessarily depend on libcrypto (for sha256 at least)?

Yes that should be right. I think I just deluded myself because I didn't want to update the mermaid diagram. Updated now though.

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1424337693)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423227364

> It seems odd to put this in "node" when `TransactionErrorString(TransactionError error)` is in "common" ?
>
> `TransactionError` is returned by `CombinePSBTs()` which seems like something that could reasonably be exposed by a command line tool without needing a full node setup.
>
> So putting this in common seems more sensible to me?

This is sort of a borderline case, and I did consider whether to move it in t
...

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1852688371)
re: https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1851506064

> Maybe removing `sock.cpp` is a bad example, since it is already impossible to link against it. `sock.cpp` uses modules that are already not part of the kernel, so trying to link it in will result in errors. Then again, if `sock.cpp` uses common modules, why is it in `util` in the first place?

I'm actually not sure what is this referring to. Running `nm ./src/util/libbitcoin_util_a-sock.o --undefined-only` I see soc
...

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424495073)
It's essentially an existing no-op. IIRC `AcceptMultipleTransactions` is only called in *unit tests* with size 1, otherwise it's always > 1.

💬 hebasto commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852691455)
Oops... Forgot to update a version in the `AX_BOOST_BASE` macro.

Fixed now.

Sorry.

💬 fanquake commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852691556)
> This one could be fixed alternatively (in theory) by nuking signals2, but I don't think this will happen any time soon.

I know a [WIP] branch for this "exists" in some form. cc @theuni

🤔 ryanofsky reviewed a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1777495734)
Updated eaf915d61d470372e63f41f11d6a873c1936f73f -> 6db04be102807ee0120981a9b8de62a55439dabb ([`pr/noshut.22`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.22) -> [`pr/noshut.23`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.23), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/noshut.22..pr/noshut.23)) just making suggested code tweaks and updating all commit messages to say what behavior is changing.

re: https://github.com/bitcoin/bitcoin/pull/28051#pullrequestr
...

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1424008835)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423738990

> same

I'd give the same answer (https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423738157) here too

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1424008653)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423738157

> [ae66429](https://github.com/bitcoin/bitcoin/commit/ae6642973dead188514aceec30ae5d73404ebcdf): Is it required to change the behavior here? Why not keep the abort?

In general, I don't think dumping core is a good way to handle errors. I think it can be a reasonable thing to do if inconsistent internal state is detected or if a precondition is violated, and halting right away will provide better debug information. Or
...

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1424008398)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423755089

> nit in [eaf915d](https://github.com/bitcoin/bitcoin/commit/eaf915d61d470372e63f41f11d6a873c1936f73f): Can drop the `()` around `Assert()`?

Good catch, fixed

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1424007995)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423753937

> nit in [eaf915d](https://github.com/bitcoin/bitcoin/commit/eaf915d61d470372e63f41f11d6a873c1936f73f): "_the_ main thread"

Fixed, thank you!

👍 ryanofsky approved a pull request: "depends: fix libmultiprocess build on aarch64"
(https://github.com/bitcoin/bitcoin/pull/28846#pullrequestreview-1778320393)
Code review ACK bde8d63b17637c507a543cebe90f2998b5847373. Only changes since last review were reverting the native_capnp change as suggested, and changing the order of the first two commits.

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1852711192)
Re https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1852688371

> I'm actually not sure what is this referring to. Running nm ./src/util/libbitcoin_util_a-sock.o --undefined-only I see sock.cpp is using LogInstance, NodeClock, and SysErrorString which should all be part of the util library.

Thanks for actually verifying, I just assumed that the `common/system.h`include was actually used :(.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1852712791)
I guess I didn't catch them all locally. Pushed a change to the last (linter commit). Also fixed the broken noise handshake test. Replaced `usleep` with `UninterruptibleSleep` to keep Windows happy.

@hebasto any idea how to handle `D:\a\bitcoin\bitcoin\src\common\sv2_noise.h(92,13): error C3646: 'WriteMsg': unknown override specifier `?

```cpp
class Sv2CipherState
{
public:
Sv2CipherState() = default;
...
ssize_t WriteMsg(Span<std::byte> msg);
```

💬 maflcko commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852715761)
Should be fine to squash a 2-line diff into one commit?

💬 hebasto commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1852717697)
> Should be fine to squash a 2-line diff into one commit?

Sure :)