💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1428012937)
There isn't a behavior change here. `StartExtraBlockRelayPeers()` can only be executed once, then it runs for the entire software lifetime.
I don't think the base condition is different. Both action paths depend on the proximity to the tip. The extra block-relay-only peers functionality is activated when the node is close to the tip (20 blocks away). And the "desirable service flags" is modified on the opposite direction, when the node detect it is further than 288 blocks from the tip.
I
...
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1428012937)
There isn't a behavior change here. `StartExtraBlockRelayPeers()` can only be executed once, then it runs for the entire software lifetime.
I don't think the base condition is different. Both action paths depend on the proximity to the tip. The extra block-relay-only peers functionality is activated when the node is close to the tip (20 blocks away). And the "desirable service flags" is modified on the opposite direction, when the node detect it is further than 288 blocks from the tip.
I
...
💬 maflcko commented on pull request "refactor: Print verbose serialize compiler error messages":
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1428027127)
Shouldn't `WritableStream` be applied at all implementations instead to constrain them? Otherwise, one could still call `Object{}.Serialize(WrongStream{})` in other parts of the code?
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1428027127)
Shouldn't `WritableStream` be applied at all implementations instead to constrain them? Otherwise, one could still call `Object{}.Serialize(WrongStream{})` in other parts of the code?
💬 maflcko commented on pull request "refactor: Print verbose serialize compiler error messages":
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1428027208)
Thanks,
* added "s"
* Left it as `BasicByte` for now, to mirror the stdlib naming: https://en.cppreference.com/w/cpp/concepts/floating_point
* Added clang-format fields
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1428027208)
Thanks,
* added "s"
* Left it as `BasicByte` for now, to mirror the stdlib naming: https://en.cppreference.com/w/cpp/concepts/floating_point
* Added clang-format fields
💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1428034455)
Since we're resetting all constraints on every loop, is this not overly verbose? (+below)
<details>
<summary>git diff on 364456f659</summary>
```diff
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 1400fadea0..7ddc45c4b4 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -200,17 +200,10 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
// The sum of the values of all spendable outpoints
constexpr CAmount SUPPLY_TO
...
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1428034455)
Since we're resetting all constraints on every loop, is this not overly verbose? (+below)
<details>
<summary>git diff on 364456f659</summary>
```diff
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 1400fadea0..7ddc45c4b4 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -200,17 +200,10 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
// The sum of the values of all spendable outpoints
constexpr CAmount SUPPLY_TO
...
💬 instagibbs commented on pull request "tests: Don't depend on value of DEFAULT_PERMIT_BAREMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858033912)
crACK https://github.com/bitcoin/bitcoin/pull/29088/commits/7b45744df33c6a4759eae1a3984f389cbac837c2
(someone should probably switch it default false and check)
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858033912)
crACK https://github.com/bitcoin/bitcoin/pull/29088/commits/7b45744df33c6a4759eae1a3984f389cbac837c2
(someone should probably switch it default false and check)
💬 ajtowns commented on pull request "refactor: Print verbose serialize compiler error messages":
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1428102823)
Yeah, that's probably reasonable
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1428102823)
Yeah, that's probably reasonable
📝 ajtowns opened a pull request: "NOMERGE UFG Default permitbaremultisig=0"
(https://github.com/bitcoin/bitcoin/pull/29093)
Cherry-picks patches from #28217 on top of #29088 so that CI can verify 29088 does what it says on the box
(https://github.com/bitcoin/bitcoin/pull/29093)
Cherry-picks patches from #28217 on top of #29088 so that CI can verify 29088 does what it says on the box
💬 maflcko commented on pull request "tests: Don't depend on value of DEFAULT_PERMIT_BAREMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858091384)
> (someone should probably switch it default false and check)
done with
```dif
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 6a7980c312..42359f12a5 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -36,7 +36,7 @@ static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
/** Default for -bytespersigop */
static constexpr unsigned int DEFAULT_BYTES_PER_SIGOP{20};
/** Default for -permitbaremultisig */
-static constexpr bool DEFAULT_PERMIT_BAREM
...
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858091384)
> (someone should probably switch it default false and check)
done with
```dif
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 6a7980c312..42359f12a5 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -36,7 +36,7 @@ static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
/** Default for -bytespersigop */
static constexpr unsigned int DEFAULT_BYTES_PER_SIGOP{20};
/** Default for -permitbaremultisig */
-static constexpr bool DEFAULT_PERMIT_BAREM
...
📝 maflcko opened a pull request: " ci: Better tidy errors "
(https://github.com/bitcoin/bitcoin/pull/29094)
(https://github.com/bitcoin/bitcoin/pull/29094)
💬 ajtowns commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1858134567)
> > Having `std::span<>` with the size as part of the type rather than a runtime thing could be nice for the various crypto things where we currently do things like `assert(key.size() == KEYLEN)`.
>
> I guess this requires a helper method to safely convert to a fixed-size span, internally preferring a compile-time check, or falling back to an assert, if it isn't available?
It doesn't /require/ it, but it'd probably be sensible so we always get an assertion failure rather than corrupted mem
...
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1858134567)
> > Having `std::span<>` with the size as part of the type rather than a runtime thing could be nice for the various crypto things where we currently do things like `assert(key.size() == KEYLEN)`.
>
> I guess this requires a helper method to safely convert to a fixed-size span, internally preferring a compile-time check, or falling back to an assert, if it isn't available?
It doesn't /require/ it, but it'd probably be sensible so we always get an assertion failure rather than corrupted mem
...
💬 ajtowns commented on pull request "tests: Don't depend on value of DEFAULT_PERMIT_BAREMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858148602)
> crACK [7b45744](https://github.com/bitcoin/bitcoin/commit/7b45744df33c6a4759eae1a3984f389cbac837c2)
>
> (someone should probably switch it default false and check)
see also CI on #29093
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858148602)
> crACK [7b45744](https://github.com/bitcoin/bitcoin/commit/7b45744df33c6a4759eae1a3984f389cbac837c2)
>
> (someone should probably switch it default false and check)
see also CI on #29093
💬 maflcko commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1858164589)
> assert(N <= src.size());
Right, seems fine to keep the assert. I was just thinking that it would be nice to drop it if the compiler can prove it isn't needed. Currently that only works for std::array or `[]` types. However, it doesn't seem to work out of the box for array wrappers, like uint256, whose extent is known at compile time.
Ideally, this should work out of the box, but I guess it is too late to change the language now, so it may be better if we keep writing our own span imp
...
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1858164589)
> assert(N <= src.size());
Right, seems fine to keep the assert. I was just thinking that it would be nice to drop it if the compiler can prove it isn't needed. Currently that only works for std::array or `[]` types. However, it doesn't seem to work out of the box for array wrappers, like uint256, whose extent is known at compile time.
Ideally, this should work out of the box, but I guess it is too late to change the language now, so it may be better if we keep writing our own span imp
...
🤔 fjahr reviewed a pull request: "doc: Add multiprocess design doc"
(https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1782842554)
ACK 837c53d14f24924cdcb2cfd8b18915882dc3b620
For me this can already be merged as is but I also have a list of nits if you retouch. I have made myself familiar multiprocess a while ago but not looked at it recently. For me this was a nice refresher and I definitely think it's helpful for newcomers.
A few more ideas of what might be interesting to add, but they are not essential and may also be done in a follow-up or just be ignored:
- Could mention potential security implications (or lack
...
(https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1782842554)
ACK 837c53d14f24924cdcb2cfd8b18915882dc3b620
For me this can already be merged as is but I also have a list of nits if you retouch. I have made myself familiar multiprocess a while ago but not looked at it recently. For me this was a nice refresher and I definitely think it's helpful for newcomers.
A few more ideas of what might be interesting to add, but they are not essential and may also be done in a follow-up or just be ignored:
- Could mention potential security implications (or lack
...
💬 fjahr commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427363274)
nit
```suggestion
- `bitcoin-node`: Manages the P2P node, indexes, and JSON-RPC server.
```
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427363274)
nit
```suggestion
- `bitcoin-node`: Manages the P2P node, indexes, and JSON-RPC server.
```
💬 fjahr commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427365136)
nit suggestions:
- 'central to the bitcoin network' isn't really relevant to this doc
- I hear people complain that big changes are always a security risk in the short term all the time
```suggestion
The Bitcoin Core software has historically employed a monolithic architecture. The existing design has integrated functionality like P2P network operations, wallet management, and a GUI into a single executable. While effective, it has limitations in flexibility, security, and scalability. Thi
...
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427365136)
nit suggestions:
- 'central to the bitcoin network' isn't really relevant to this doc
- I hear people complain that big changes are always a security risk in the short term all the time
```suggestion
The Bitcoin Core software has historically employed a monolithic architecture. The existing design has integrated functionality like P2P network operations, wallet management, and a GUI into a single executable. While effective, it has limitations in flexibility, security, and scalability. Thi
...
💬 fjahr commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428162298)
nit: typo
```suggestion
- **Handling Special Cases**: The `mpgen` tool and `libmultiprocess` library can convert most C++ types to and from Cap’n Proto types automatically, including interface types, primitive C++ types, standard C++ types like `std::vector`, `std::set`, `std::map`, `std::tuple`, and `std::function`, as well as simple C++ structs that consist of aforementioned types and whose fields correspond 1:1 with Cap’n Proto struct fields. For other types, `*-types.h` files provide custo
...
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428162298)
nit: typo
```suggestion
- **Handling Special Cases**: The `mpgen` tool and `libmultiprocess` library can convert most C++ types to and from Cap’n Proto types automatically, including interface types, primitive C++ types, standard C++ types like `std::vector`, `std::set`, `std::map`, `std::tuple`, and `std::function`, as well as simple C++ structs that consist of aforementioned types and whose fields correspond 1:1 with Cap’n Proto struct fields. For other types, `*-types.h` files provide custo
...
💬 fjahr commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428164608)
```suggestion
The libmultiprocess runtime is designed to place as few constraints as possible on IPC interfaces and make IPC calls act like normal function calls. Method arguments, return values, and exceptions are automatically serialized and sent between processes. Object references and `std::function` arguments are automatically tracked and mapped to allow invoked code to call back into invoking code at any time. And there is a 1:1 threading model where every client thread has a correspondin
...
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428164608)
```suggestion
The libmultiprocess runtime is designed to place as few constraints as possible on IPC interfaces and make IPC calls act like normal function calls. Method arguments, return values, and exceptions are automatically serialized and sent between processes. Object references and `std::function` arguments are automatically tracked and mapped to allow invoked code to call back into invoking code at any time. And there is a 1:1 threading model where every client thread has a correspondin
...
💬 fjahr commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428154601)
nit: This just repeats what is said in the Introduction part and isn't really needed for the purpose of this doc IMO
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428154601)
nit: This just repeats what is said in the Introduction part and isn't really needed for the purpose of this doc IMO
💬 fjahr commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428165515)
```suggestion
In the current design, class names, method names, and parameter names are duplicated between C++ interfaces in [`src/interfaces/`](../../src/interfaces/) and Cap’n Proto files in [`src/ipc/capnp/`](../../src/ipc/capnp/). While this keeps C++ interface headers simple and free of references to IPC, it is a maintenance burden because it means inconsistencies between C++ declarations and Cap’n Proto declarations will result in compile errors. (Static type checking ensures these are no
...
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428165515)
```suggestion
In the current design, class names, method names, and parameter names are duplicated between C++ interfaces in [`src/interfaces/`](../../src/interfaces/) and Cap’n Proto files in [`src/ipc/capnp/`](../../src/ipc/capnp/). While this keeps C++ interface headers simple and free of references to IPC, it is a maintenance burden because it means inconsistencies between C++ declarations and Cap’n Proto declarations will result in compile errors. (Static type checking ensures these are no
...
💬 fjahr commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428187716)
Maybe mention that his is based on code that is not merged yet, for example `chain.capnp` is linked below but it doesn't exist on master yet.
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428187716)
Maybe mention that his is based on code that is not merged yet, for example `chain.capnp` is linked below but it doesn't exist on master yet.