💬 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.
💬 fjahr commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428168964)
```suggestion
The multiprocess feature requires [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) as dependencies. A simple way to get started using it without installing these dependencies manually is to use the [depends system](../depends) with the `MULTIPROCESS=1` [dependency option](../depends#dependency-options) passed to make:
```
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428168964)
```suggestion
The multiprocess feature requires [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) as dependencies. A simple way to get started using it without installing these dependencies manually is to use the [depends system](../depends) with the `MULTIPROCESS=1` [dependency option](../depends#dependency-options) passed to make:
```
💬 jrakibi commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1858178944)
> Hey, @maflcko @jrakibi Is there anyone working on this issue? If not I’d like to pick it up. Thanks
Hey @mohamedawnallah , I am currently preoccupied with other assignments, feel free to take it over
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1858178944)
> Hey, @maflcko @jrakibi Is there anyone working on this issue? If not I’d like to pick it up. Thanks
Hey @mohamedawnallah , I am currently preoccupied with other assignments, feel free to take it over
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428218062)
Not 100% sure how to encapsulate this exactly since Workspace is private to MemPoolAccept, which is private to validation. We have some tests that the ancestor sets are correctly calculated. Maybe we add a helper function? Or I can add some more assumes.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428218062)
Not 100% sure how to encapsulate this exactly since Workspace is private to MemPoolAccept, which is private to validation. We have some tests that the ancestor sets are correctly calculated. Maybe we add a helper function? Or I can add some more assumes.
🤔 glozow reviewed a pull request: "tests: Don't depend on value of DEFAULT_PERMIT_BAREMULTISIG"
(https://github.com/bitcoin/bitcoin/pull/29088#pullrequestreview-1784568242)
ACK 7b45744df33c6a4759eae1a3984f389cbac837c2, changed default locally and all tests passed
(https://github.com/bitcoin/bitcoin/pull/29088#pullrequestreview-1784568242)
ACK 7b45744df33c6a4759eae1a3984f389cbac837c2, changed default locally and all tests passed
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428100898)
```suggestion
if (ws.m_ptx->nVersion == 3) {
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428100898)
```suggestion
if (ws.m_ptx->nVersion == 3) {
```
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428105177)
would be nice to explain if the key a v3 tx or the ancestor set is, for name reading purposes
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428105177)
would be nice to explain if the key a v3 tx or the ancestor set is, for name reading purposes
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428172941)
txns can not be empty
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428172941)
txns can not be empty
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428129715)
```suggestion
const int64_t vsize = GetVirtualTransactionSize(*tx, /*nSigOpCost=*/0, /*bytes_per_sigop=*/0);
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428129715)
```suggestion
const int64_t vsize = GetVirtualTransactionSize(*tx, /*nSigOpCost=*/0, /*bytes_per_sigop=*/0);
```
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428162185)
Anything that's not-already `PreCheck`'ed will necessarily have empty `Workspace::m_ancestors` sets, so there will be a mismatch in the over-estimates.
(not sure it matters, just noting)
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428162185)
Anything that's not-already `PreCheck`'ed will necessarily have empty `Workspace::m_ancestors` sets, so there will be a mismatch in the over-estimates.
(not sure it matters, just noting)
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428144170)
```suggestion
// Map from txid of a V3 transaction to its in-package ancestor set. Not MemPoolAccept-wide
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1428144170)
```suggestion
// Map from txid of a V3 transaction to its in-package ancestor set. Not MemPoolAccept-wide
```