🤔 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
...
(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
(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
...
(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
(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!
(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.
(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 :(.
(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);
```
(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?
(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 :)
(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
(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
(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
...
(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.
(https://github.com/bitcoin/bitcoin/pull/29068)
Currently python unit test failures are ignored.
Fix this.
💬 maflcko commented on pull request "test: Actually fail when a python unit test fails":
(https://github.com/bitcoin/bitcoin/pull/29068#issuecomment-1852744258)
Can be tested with something like:
```diff
diff --git a/test/functional/test_framework/crypto/bip324_cipher.py b/test/functional/test_framework/crypto/bip324_cipher.py
index 56190647f2..56211e231d 100644
--- a/test/functional/test_framework/crypto/bip324_cipher.py
+++ b/test/functional/test_framework/crypto/bip324_cipher.py
@@ -89,7 +89,7 @@ class FSChaCha20Poly1305:
# Test vectors from RFC8439 consisting of plaintext, aad, 32 byte key, 12 byte nonce and ciphertext
AEAD_TESTS = [
...
(https://github.com/bitcoin/bitcoin/pull/29068#issuecomment-1852744258)
Can be tested with something like:
```diff
diff --git a/test/functional/test_framework/crypto/bip324_cipher.py b/test/functional/test_framework/crypto/bip324_cipher.py
index 56190647f2..56211e231d 100644
--- a/test/functional/test_framework/crypto/bip324_cipher.py
+++ b/test/functional/test_framework/crypto/bip324_cipher.py
@@ -89,7 +89,7 @@ class FSChaCha20Poly1305:
# Test vectors from RFC8439 consisting of plaintext, aad, 32 byte key, 12 byte nonce and ciphertext
AEAD_TESTS = [
...
💬 maflcko commented on pull request "test: Actually fail when a python unit test fails":
(https://github.com/bitcoin/bitcoin/pull/29068#issuecomment-1852746032)
Fun fact: This problem would not exit in a type-safe language.
(https://github.com/bitcoin/bitcoin/pull/29068#issuecomment-1852746032)
Fun fact: This problem would not exit in a type-safe language.
💬 aureleoules commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1852759253)
This pull does fix the crash but I am still getting valgrind errors when executing the benchmark, which did not happen on master before. I haven't verified that these errors were introduced by #25273 but I would suppose so.
```
valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
==243376== Memcheck, a memory error detector
==243376== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==243376== Using Valgrind-3.18.1 and LibVEX; rerun with
...
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1852759253)
This pull does fix the crash but I am still getting valgrind errors when executing the benchmark, which did not happen on master before. I haven't verified that these errors were introduced by #25273 but I would suppose so.
```
valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
==243376== Memcheck, a memory error detector
==243376== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==243376== Using Valgrind-3.18.1 and LibVEX; rerun with
...
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424551588)
Pushed a functional test covering this case specifically which is rejected due to existence of mempool ancestors.
I'll try and rephrase soon.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1424551588)
Pushed a functional test covering this case specifically which is rejected due to existence of mempool ancestors.
I'll try and rephrase soon.
💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1424554648)
I guess this code will never be hit, so it doesn't matter much. If it was hit, I don't see how the user would shutdown the process without an unclean termination, so might as well do an abort. Generally, it should be fine to abort Bitcoin Core at any time, because any critical data should always be flushed atomically to storage.
But as this code should never be hit, it also seems fine to give the user the option to call RPCs to unload wallets, flush the chainstate, and disconnect peers, etc,
...
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1424554648)
I guess this code will never be hit, so it doesn't matter much. If it was hit, I don't see how the user would shutdown the process without an unclean termination, so might as well do an abort. Generally, it should be fine to abort Bitcoin Core at any time, because any critical data should always be flushed atomically to storage.
But as this code should never be hit, it also seems fine to give the user the option to call RPCs to unload wallets, flush the chainstate, and disconnect peers, etc,
...
💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1424568910)
I also don't see that as a real risk, but I do see the problem is that whoever reads this code (or decides to use `ForEachNode` somewhere else) could easily get confused, especially since some callers check again for `fDisconnect`.
I added a refactor commit renaming `ForEachNode()` to `ForEachFullyConnectedNode()` and removing the duplicate checks / comments. What do you think about that approach?
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1424568910)
I also don't see that as a real risk, but I do see the problem is that whoever reads this code (or decides to use `ForEachNode` somewhere else) could easily get confused, especially since some callers check again for `fDisconnect`.
I added a refactor commit renaming `ForEachNode()` to `ForEachFullyConnectedNode()` and removing the duplicate checks / comments. What do you think about that approach?