💬 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.

📝 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
...

👍 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.

📝 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
...

⚠️ 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 |
...

💬 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
...

👍 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.

🤔 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.

💬 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.

💬 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.

💬 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
...

💬 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:
...

💬 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?

💬 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);
| ^~~~~~~~~~~~~~~
|
...

💬 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?

💬 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.)

🤔 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
```

💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333521)
> I wonder if this can be generalized to pass test-only options (along with a helper HasTestOption, similar to IsDeprecatedRPCEnabled?

what would a function like `HasTestOption` do though? we could introduce it here to check if we're using a known test option("relay", "addrman"). even if we use unknown test options, we don't need to throw an error right.

> In any case, (default: %s)", "relay" is wrong, no?

done.

💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333665)
done.

💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333746)
done.