💬 maflcko commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1417977573)
Is this commit needed? I guess there could be issues if the same translation unit is compiled twice?
Not sure if Hex is correct to put into "crypto".
If this isn't needed, maybe leave this for a follow-up?
An alternative would be to just remove it from the util library and keep it only in consensus?
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1417977573)
Is this commit needed? I guess there could be issues if the same translation unit is compiled twice?
Not sure if Hex is correct to put into "crypto".
If this isn't needed, maybe leave this for a follow-up?
An alternative would be to just remove it from the util library and keep it only in consensus?
🤔 pablomartin4btc reviewed a pull request: "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block"
(https://github.com/bitcoin/bitcoin/pull/26415#pullrequestreview-1768668905)
Concept ACK
I'd like to run the bench using #26684 which I have pending of reviewing.
(https://github.com/bitcoin/bitcoin/pull/26415#pullrequestreview-1768668905)
Concept ACK
I'd like to run the bench using #26684 which I have pending of reviewing.
💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1418007227)
> My point is that for developers, we already have tracepoints as a tool for debugging and observability into low-level details of how the software is working. In many cases, a developer will try to reproduce an issue reported by a user on their own node, at which point debuggers and tracepoints are going to be more useful than logs.
Replicate issues without information is by far more challenging than replicating them with information. It is a blind guess.
Also, what you are saying works (
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1418007227)
> My point is that for developers, we already have tracepoints as a tool for debugging and observability into low-level details of how the software is working. In many cases, a developer will try to reproduce an issue reported by a user on their own node, at which point debuggers and tracepoints are going to be more useful than logs.
Replicate issues without information is by far more challenging than replicating them with information. It is a blind guess.
Also, what you are saying works (
...
💬 TheCharlatan commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1418034290)
Thanks for the rationale, I indeed did not think of the future IPC delay. If we only start processing events after `app.exec()`, why not move this to just before `registerShutdownBlockReason`? Looking at the history it looks to me like this was put where it is to be in the general vicinity of the `installEvent*` calls.
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1418034290)
Thanks for the rationale, I indeed did not think of the future IPC delay. If we only start processing events after `app.exec()`, why not move this to just before `registerShutdownBlockReason`? Looking at the history it looks to me like this was put where it is to be in the general vicinity of the `installEvent*` calls.
📝 ryanofsky opened a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015)
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of
...
(https://github.com/bitcoin/bitcoin/pull/29015)
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of
...
👍 TheCharlatan approved a pull request: "build: use macOS 14 SDK (Xcode 15.0)"
(https://github.com/bitcoin/bitcoin/pull/28622#pullrequestreview-1768741962)
ACK 8ea45e626e5a0482ee11d4376f961d8126ce7c7b
Get the same build hashes and SDK hashes. Ran the binary on macOS 11.1. Completed a depends build on my linux dev box. Verified the boost process patch.
(https://github.com/bitcoin/bitcoin/pull/28622#pullrequestreview-1768741962)
ACK 8ea45e626e5a0482ee11d4376f961d8126ce7c7b
Get the same build hashes and SDK hashes. Ran the binary on macOS 11.1. Completed a depends build on my linux dev box. Verified the boost process patch.
📝 niftynei opened a pull request: "RPC: add new `listmempooltransactions`"
(https://github.com/bitcoin/bitcoin/pull/29016)
## Proposed Update
Add a new RPC endpoint, `listmempooltransactions`. Takes as args a `start_sequence` and `verbose`.
Returns:
- if verbose false, list of txids + their `entry_sequence` where each entry's `entry_sequence` >= the provided `start_sequence.
- if verbose true, raw tx output info including each entry's `entry_sequence`.
Builds on work done in #19572.
## Rationale
The current mempool RPCs are lacking an ability to scan for updates in a more efficient manner. You can
...
(https://github.com/bitcoin/bitcoin/pull/29016)
## Proposed Update
Add a new RPC endpoint, `listmempooltransactions`. Takes as args a `start_sequence` and `verbose`.
Returns:
- if verbose false, list of txids + their `entry_sequence` where each entry's `entry_sequence` >= the provided `start_sequence.
- if verbose true, raw tx output info including each entry's `entry_sequence`.
Builds on work done in #19572.
## Rationale
The current mempool RPCs are lacking an ability to scan for updates in a more efficient manner. You can
...
⚠️ centaur1 opened an issue: "Build error on Ubuntu 22.04.3 LTS"
(https://github.com/bitcoin/bitcoin/issues/29017)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Building on 22.04.3 LTS (GNU/Linux 5.15.0-87-generic x86_64) results in this error:
test/base58_tests.cpp: In member function ‘void base58_tests::base58_EncodeBase58::test_method()’:
test/base58_tests.cpp:26:44: error: invalid initialization of reference of type ‘const string&’ {aka ‘const std::__cxx11::basic_string<char>&’} from expression of type ‘const unsigned char [1462]’
26 |
...
(https://github.com/bitcoin/bitcoin/issues/29017)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Building on 22.04.3 LTS (GNU/Linux 5.15.0-87-generic x86_64) results in this error:
test/base58_tests.cpp: In member function ‘void base58_tests::base58_EncodeBase58::test_method()’:
test/base58_tests.cpp:26:44: error: invalid initialization of reference of type ‘const string&’ {aka ‘const std::__cxx11::basic_string<char>&’} from expression of type ‘const unsigned char [1462]’
26 |
...
💬 ryanofsky commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418057188)
re: https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1417977573
> Is this commit needed? I guess there could be issues if the same translation unit is compiled twice?
>
> Not sure if Hex is correct to put into "crypto".
>
> If this isn't needed, maybe leave this for a follow-up?
>
> An alternative would be to just remove it from the util library and keep it only in consensus?
Not sure about the current state, but when this was asked previously https://github.com/bitcoin/b
...
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418057188)
re: https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1417977573
> Is this commit needed? I guess there could be issues if the same translation unit is compiled twice?
>
> Not sure if Hex is correct to put into "crypto".
>
> If this isn't needed, maybe leave this for a follow-up?
>
> An alternative would be to just remove it from the util library and keep it only in consensus?
Not sure about the current state, but when this was asked previously https://github.com/bitcoin/b
...
👍 ryanofsky approved a pull request: "build: Introduce internal kernel library"
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1768762282)
Code review ACK 34970bde23dd87fc0fea5a1970880761f7184774.
Main change since last review is no longer moving the spanparsing.h file here, to avoid making util depend on common. I have since opened a separate PR #29015 to remove spanparsing.h functions from util by splitting it up.
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1768762282)
Code review ACK 34970bde23dd87fc0fea5a1970880761f7184774.
Main change since last review is no longer moving the spanparsing.h file here, to avoid making util depend on common. I have since opened a separate PR #29015 to remove spanparsing.h functions from util by splitting it up.
🤔 mzumsande reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1768762854)
ACK f479a99cc92c7966266804e8c33338cebbc44fbc
I have reviewed the code. I did some testing / gained more confidence that it works well by having a [branch](https://github.com/mzumsande/bitcoin/tree/tmp_bip324_fixalltests) where, with some adjustments, the python v2 p2p module is used for the entire functional test suite.
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1768762854)
ACK f479a99cc92c7966266804e8c33338cebbc44fbc
I have reviewed the code. I did some testing / gained more confidence that it works well by having a [branch](https://github.com/mzumsande/bitcoin/tree/tmp_bip324_fixalltests) where, with some adjustments, the python v2 p2p module is used for the entire functional test suite.
💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1418061039)
One thing to consider is that now we have two locks for p2p, we need to worry about deadlocks.
There is an implied lock order, `on_message` locks `p2p_lock`, and can call messages (e.g. `on_inv`) that will call `send_message` and lock `_send_lock`. So we must make sure never to take `p2p_lock` after `_send_lock`. I don't think that this is currently possible, but maybe we should add a comment about this.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1418061039)
One thing to consider is that now we have two locks for p2p, we need to worry about deadlocks.
There is an implied lock order, `on_message` locks `p2p_lock`, and can call messages (e.g. `on_inv`) that will call `send_message` and lock `_send_lock`. So we must make sure never to take `p2p_lock` after `_send_lock`. I don't think that this is currently possible, but maybe we should add a comment about this.
💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418067302)
Re https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1417977573
> An alternative would be to just remove it from the util library and keep it only in consensus?
These functions are used by components of `bitcoin-cli`. The `bitcoin-cli` binary should not have to rely on the consensus library, so I don't think that is an alternative.
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418067302)
Re https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1417977573
> An alternative would be to just remove it from the util library and keep it only in consensus?
These functions are used by components of `bitcoin-cli`. The `bitcoin-cli` binary should not have to rely on the consensus library, so I don't think that is an alternative.
💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1418077030)
> Thanks for the rationale, I indeed did not think of the future IPC delay. If we only start processing events after `app.exec()`, why not move this to just before `registerShutdownBlockReason`? Looking at the history it looks to me like this was put where it is to be in the general vicinity of the `installEvent*` calls.
I can make this change if you still think it's a good idea after this, but I think it's reasonable to want to keep the installEvent calls together. I also didn't want to just
...
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1418077030)
> Thanks for the rationale, I indeed did not think of the future IPC delay. If we only start processing events after `app.exec()`, why not move this to just before `registerShutdownBlockReason`? Looking at the history it looks to me like this was put where it is to be in the general vicinity of the `installEvent*` calls.
I can make this change if you still think it's a good idea after this, but I think it's reasonable to want to keep the installEvent calls together. I also didn't want to just
...
💬 sipsorcery commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1843853088)
Maybe something in txmempool.h that the msvc compiler doesn't like?
```
D:\a\bitcoin\bitcoin\src\txmempool.h(457,60): error C2143: syntax error: missing ')' before 'public' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
D:\a\bitcoin\bitcoin\src\txmempool.h(457,60): error C2143: syntax error: missing ';' before 'public' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
D:\a\bitcoin\bitcoin\src\txmempool.h(457,67): error C2059: syntax error: ')' [D:
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1843853088)
Maybe something in txmempool.h that the msvc compiler doesn't like?
```
D:\a\bitcoin\bitcoin\src\txmempool.h(457,60): error C2143: syntax error: missing ')' before 'public' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
D:\a\bitcoin\bitcoin\src\txmempool.h(457,60): error C2143: syntax error: missing ';' before 'public' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
D:\a\bitcoin\bitcoin\src\txmempool.h(457,67): error C2059: syntax error: ')' [D:
...
💬 ajtowns commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1843941916)
> The cumulative size of the individual binaries (compiled with LTO) is 3.6GB vs 14GB when search and replacing `std::getenv("FUZZ")` (like we do in oss-fuzz).
What does this mean? I'm seeing file sizes of 448MB (with fuzzing enabled) and 146MB (with a normal build) for test/fuzz/fuzz.
This seems like it's reverting #20560?
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1843941916)
> The cumulative size of the individual binaries (compiled with LTO) is 3.6GB vs 14GB when search and replacing `std::getenv("FUZZ")` (like we do in oss-fuzz).
What does this mean? I'm seeing file sizes of 448MB (with fuzzing enabled) and 146MB (with a normal build) for test/fuzz/fuzz.
This seems like it's reverting #20560?
💬 kevkevinpal commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#discussion_r1418224065)
looks like the tidy ci is getting upset here
```
rpc/mempool.cpp:462:82: error: argument name 'inclue_hex' in comment does not match parameter name 'include_hex' [bugprone-argument-comment,-warnings-as-errors]
462 | TxToUniv(e.GetTx(), /*block_hash=*/uint256::ZERO, /*entry=*/txentry, /*inclue_hex=*/false);
| ^~~~~~~~~~~~~~~
|
...
(https://github.com/bitcoin/bitcoin/pull/29016#discussion_r1418224065)
looks like the tidy ci is getting upset here
```
rpc/mempool.cpp:462:82: error: argument name 'inclue_hex' in comment does not match parameter name 'include_hex' [bugprone-argument-comment,-warnings-as-errors]
462 | TxToUniv(e.GetTx(), /*block_hash=*/uint256::ZERO, /*entry=*/txentry, /*inclue_hex=*/false);
| ^~~~~~~~~~~~~~~
|
...
💬 ajtowns commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1844180519)
> Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications.
Is the idea here that `util` should be for generic code, and that these bits and pieces (like fees and error) are specific bitcoin features that are only relevant to wallet/p2p, so aren't generic?
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1844180519)
> Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications.
Is the idea here that `util` should be for generic code, and that these bits and pieces (like fees and error) are specific bitcoin features that are only relevant to wallet/p2p, so aren't generic?
💬 sdaftuar commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418261403)
Is there a reason that we are invoking `GetSortedDepthAndScore()` here, ie do any of the callers rely on a particular sort order?
(I'm planning to eliminate this function in #28676, so just want to understand if this behavior needs to be preserved for some reason.)
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418261403)
Is there a reason that we are invoking `GetSortedDepthAndScore()` here, ie do any of the callers rely on a particular sort order?
(I'm planning to eliminate this function in #28676, so just want to understand if this behavior needs to be preserved for some reason.)
🤔 pablomartin4btc reviewed a pull request: "RPC: add new `listmempooltransactions`"
(https://github.com/bitcoin/bitcoin/pull/29016#pullrequestreview-1769056893)
Also, [missing](https://github.com/bitcoin/bitcoin/pull/29016/checks?check_run_id=19389652409) test coverage for the new command:
```
Uncovered RPC commands:
- listmempooltransactions
```
(https://github.com/bitcoin/bitcoin/pull/29016#pullrequestreview-1769056893)
Also, [missing](https://github.com/bitcoin/bitcoin/pull/29016/checks?check_run_id=19389652409) test coverage for the new command:
```
Uncovered RPC commands:
- listmempooltransactions
```