💬 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 :)

💬 achow101 commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1852720898)
ACK 37c75c58202f89b752500f76c872d7f8caf6bdb3

👍 TheCharlatan approved a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1778364556)
Re-ACK 6db04be102807ee0120981a9b8de62a55439dabb

💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1852741252)


On December 12, 2023 5:49:19 PM GMT+01:00, Unhosted Marcellus ***@***.***> wrote:
>> > The original Satoshi client didn't allow embedding data onchain, this has always been an exploit.
>>
>> Bitcoin was released with no standardness rules at all. You absolutely could embed data onchain. You could even publish arbitrary data to the UTXO set easily.
>
>That was poor wording, but was precisely my point.
>
>Satoshi introduced OP_RETURN as a mechanism to prove that some sats were provably unspenda
...

📝 maflcko opened a pull request: "test: Actually fail when a python unit test fails"
(https://github.com/bitcoin/bitcoin/pull/29068)
Currently python unit test failures are ignored.

Fix this.