💬 maflcko commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1708917038)
As a follow-up idea, it could make sense to reduce the timeout here and instead scale it with the self.timeout_factor, so that the timeout is magically adjusted to the speed of the test-env it is running in
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1708917038)
As a follow-up idea, it could make sense to reduce the timeout here and instead scale it with the self.timeout_factor, so that the timeout is magically adjusted to the speed of the test-env it is running in
👍 maflcko approved a pull request: "tracing: network connection tracepoints"
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2227173636)
Left a minor follow-up idea and a few style nits, but nothing blocking. lgtm
review ACK edef8c78e3a436e7187937b6e1bc906392ef9891 💽
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+
...
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2227173636)
Left a minor follow-up idea and a few style nits, but nothing blocking. lgtm
review ACK edef8c78e3a436e7187937b6e1bc906392ef9891 💽
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+
...
💬 maflcko commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1708922064)
style nit in 07e7bd873bd512c9d488efbaf452e54efc92ac0a: Could clarify that this is *seconds*, not nano- or other seconds (like in other tracepoints)?
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1708922064)
style nit in 07e7bd873bd512c9d488efbaf452e54efc92ac0a: Could clarify that this is *seconds*, not nano- or other seconds (like in other tracepoints)?
⚠️ maflcko opened an issue: "test: "system_tests/run_command" unit test fails with different locale"
(https://github.com/bitcoin/bitcoin/issues/30608)
The issue from yesterday was closed (https://github.com/bitcoin/bitcoin/issues/30601#event-13795338265), so opening a new issue for this, because the workaround does not fix the issue.
The error is:
```
test/system_tests.cpp(88): error: in "system_tests/run_command": check what.find(expected) != std::string::npos has failed
```
As a temporary workaround, one can try `--disable-external-signer`, if they don't need it. Alternatively, they can set a different locale, for just the tests.
...
(https://github.com/bitcoin/bitcoin/issues/30608)
The issue from yesterday was closed (https://github.com/bitcoin/bitcoin/issues/30601#event-13795338265), so opening a new issue for this, because the workaround does not fix the issue.
The error is:
```
test/system_tests.cpp(88): error: in "system_tests/run_command": check what.find(expected) != std::string::npos has failed
```
As a temporary workaround, one can try `--disable-external-signer`, if they don't need it. Alternatively, they can set a different locale, for just the tests.
...
💬 Sjors commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2275298931)
I don't mind making a new torrent.
I'm still puzzled why all of a sudden a single integer in a serialized file is such a big deal that removing it is simpler than fixing it. But I haven't reviewed this PR yet.
It's true that we can add it back later, but it seems wasteful if we first have to review #30598 only to revert it later and still having to review the changes here.
Even if we never want it back, it seems very likely we'll an integer to some other serialized thing in the future.
...
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2275298931)
I don't mind making a new torrent.
I'm still puzzled why all of a sudden a single integer in a serialized file is such a big deal that removing it is simpler than fixing it. But I haven't reviewed this PR yet.
It's true that we can add it back later, but it seems wasteful if we first have to review #30598 only to revert it later and still having to review the changes here.
Even if we never want it back, it seems very likely we'll an integer to some other serialized thing in the future.
...
💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709002249)
> Are you able to build them using Autotools?
Yea, the steps in `fuzzing.md` currently work with master, but don't work this branch (ad2140d4d8cccec82fdad07bdc7532e202282306).
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709002249)
> Are you able to build them using Autotools?
Yea, the steps in `fuzzing.md` currently work with master, but don't work this branch (ad2140d4d8cccec82fdad07bdc7532e202282306).
💬 maflcko commented on pull request "tracing: explicitly cast block_connected duration to nanoseconds":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2275336617)
lgtm ACK 34c18de369b7bd71334f849395723f49dffae560
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2275336617)
lgtm ACK 34c18de369b7bd71334f849395723f49dffae560
💬 dergoegge commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709005443)
We probably don't.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709005443)
We probably don't.
💬 dergoegge commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709030842)
I've only tried the instructions in this doc on linux when I wrote them a few weeks back. I'm guessing this is a Mac only issue but perhaps not.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709030842)
I've only tried the instructions in this doc on linux when I wrote them a few weeks back. I'm guessing this is a Mac only issue but perhaps not.
💬 dergoegge commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709007912)
Right, the "-j$(nproc)" bit should be removed. I don't have a Mac so I don't no if these instructions are still relevant.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709007912)
Right, the "-j$(nproc)" bit should be removed. I don't have a Mac so I don't no if these instructions are still relevant.
✅ hebasto closed a pull request: "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP"
(https://github.com/bitcoin/bitcoin/pull/29790)
(https://github.com/bitcoin/bitcoin/pull/29790)
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709076520)
The mac equivalent is `make -j$(sysctl -n hw.ncpu)`
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709076520)
The mac equivalent is `make -j$(sysctl -n hw.ncpu)`
💬 gmaxwell commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709093275)
Do these adhoc initialization costs interact well with the with the upper bound 2^(k-1) and the empirical sqrt(2^k)+1 limits in the tests or should the bounds in the test be increased by these adhoc initialization costs?
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709093275)
Do these adhoc initialization costs interact well with the with the upper bound 2^(k-1) and the empirical sqrt(2^k)+1 limits in the tests or should the bounds in the test be increased by these adhoc initialization costs?
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2275425772)
Post-merge light re-utACK
Happy to see `enforce_BIP94` replace the hardcoded genesis block.
Followup suggestion: let's make `60 * 60 * 2` a constant and add a `static_assert` that it's equal to `MAX_FUTURE_BLOCK_TIME`. They are not the same thing, since `time-timewarp-attack` is (testnet4) consensus whereas `time-too-new` is not. But if we ever change one without changing the other, it can cause a chain split.
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2275425772)
Post-merge light re-utACK
Happy to see `enforce_BIP94` replace the hardcoded genesis block.
Followup suggestion: let's make `60 * 60 * 2` a constant and add a `static_assert` that it's equal to `MAX_FUTURE_BLOCK_TIME`. They are not the same thing, since `time-timewarp-attack` is (testnet4) consensus whereas `time-too-new` is not. But if we ever change one without changing the other, it can cause a chain split.
💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708937001)
I'm having a hard time reading this TODO. Can you try and reformulate it?
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708937001)
I'm having a hard time reading this TODO. Can you try and reformulate it?
💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708943992)
Typo nit: s/canproto/capnproto/ (here and elsewhere).
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708943992)
Typo nit: s/canproto/capnproto/ (here and elsewhere).
💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708942643)
Nit: it would be nice to give this the global `g_` prefix (though it is not introduced in this PR).
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708942643)
Nit: it would be nice to give this the global `g_` prefix (though it is not introduced in this PR).
💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708996040)
Not directly related to this PR, but did these type hook methods get renamed since the docs were written? https://github.com/ryanofsky/bitcoin/blob/pr/ipc/doc/design/multiprocess.md#type-hooks-in-srcipccapnp-typesh
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708996040)
Not directly related to this PR, but did these type hook methods get renamed since the docs were written? https://github.com/ryanofsky/bitcoin/blob/pr/ipc/doc/design/multiprocess.md#type-hooks-in-srcipccapnp-typesh
💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709048212)
I'm a bit confused by this file. It is only included by `mining.capnp`, but in turn includes the auto-generated header from `mining.capnp` here. What is the purpose of this? Couldn't the capnp file just directly include `interfaces/mining.h`? At least doing so compiles for me.
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709048212)
I'm a bit confused by this file. It is only included by `mining.capnp`, but in turn includes the auto-generated header from `mining.capnp` here. What is the purpose of this? Couldn't the capnp file just directly include `interfaces/mining.h`? At least doing so compiles for me.
💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709096542)
I think it would be nice to test some more invariants here:
```diff
diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp
index aabc0650b8..fa5273446f 100644
--- a/src/test/ipc_test.cpp
+++ b/src/test/ipc_test.cpp
@@ -79,0 +80,3 @@ void IpcTest()
+ BOOST_CHECK_EQUAL(bs1.IsValid(), bs2.IsValid());
+ BOOST_CHECK_EQUAL(bs1.IsError(), bs2.IsError());
+ BOOST_CHECK_EQUAL(bs1.IsInvalid(), bs2.IsInvalid());
@@ -82,0 +86,9 @@ void IpcTest()
+ BlockValidationState bs3;
+ B
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709096542)
I think it would be nice to test some more invariants here:
```diff
diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp
index aabc0650b8..fa5273446f 100644
--- a/src/test/ipc_test.cpp
+++ b/src/test/ipc_test.cpp
@@ -79,0 +80,3 @@ void IpcTest()
+ BOOST_CHECK_EQUAL(bs1.IsValid(), bs2.IsValid());
+ BOOST_CHECK_EQUAL(bs1.IsError(), bs2.IsError());
+ BOOST_CHECK_EQUAL(bs1.IsInvalid(), bs2.IsInvalid());
@@ -82,0 +86,9 @@ void IpcTest()
+ BlockValidationState bs3;
+ B
...