🤔 furszy reviewed a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1832560555)
See #28170. Obvious concept ACK.
We do have a post-IBD bug on this process.

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1900458347)
> HasAllDesirableServiceFlags(), relies on IsInitialBlockDownload() / DEFAULT_MAX_TIP_AGE (24h) on master
...
> I think this means that the safety buffer of 144 blocks proposed in [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki) would have been removed.

I do not think `-maxtipage` / `DEFAULT_MAX_TIP_AGE` implements "the safety buffer" from BIP159. The former was introduced about 2 years before BIP159, in 2015: https://github.com/bitcoin/bitcoin/pull/5987.

BIP159 r
...

💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1900470464)
Here's a godbolt test that shows how various compilers perform: https://gcc.godbolt.org/z/nTadqEP83

To test with/without builtins, comment/un-comment the `DISABLE_BUILTIN_BSWAPS` define at line 14.

From my tests:
- clang: all c++20 supporting versions (>= 10.0) optimize without the need for builtins
- gcc: no version optimizes without the builtins
- msvc: versions >= 19.37 optimize without builtins

💬 jamesob commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1900505846)
Oddly enough, even with `gcc 12.2.0`, the regression on the first machine holds. Artifacts here: https://gist.github.com/jamesob/1f4456f1f9bafcabb392210bbfe9f240

Will run the secp benches in isolation.

📝 stickies-v opened a pull request: "test: ensure output is large enough to pay for its fees"
(https://github.com/bitcoin/bitcoin/pull/29283)
Fixes a (rare) intermittency issue in wallet_import_rescan.py

Since we [use](https://github.com/bitcoin/bitcoin/blob/03752444cd54df05a731557968d5a9f33a55c55c/test/functional/wallet_import_rescan.py#L296) `subtract_fee_from_outputs=[0]` in the `send` command, the output amount must at least be as large as the fee we're paying.

Example in CI: https://api.cirrus-ci.com/v1/task/6107972259020800/logs/ci.log

```
2024-01-18T22:16:12.383000Z TestFramework (INFO): Test that the mempool is resca
...

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1900516149)
Reviewers might be interested in https://github.com/bitcoin/bitcoin/pull/29283, fixing a rare intermittency issue

💬 josibake commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#issuecomment-1900521814)
Rebased https://github.com/bitcoin/bitcoin/commit/3f4b0f77c7f2ff52b9c84bf1c5a70281a066cfe2 -> https://github.com/bitcoin/bitcoin/commit/3b0dd79cacaf4e97489746ace5c1c8d6fe8db38d ([implement-bip352-sending-01](https://github.com/josibake/bitcoin/tree/implement-bip352-sending-01) -> [implement-bip352-01-rebase](https://github.com/josibake/bitcoin/tree/implement-bip352-sending-01-rebase), [compare](https://github.com/josibake/bitcoin/compare/implement-bip352-sending-01..implement-bip352-sending-01-r
...

🤔 glozow reviewed a pull request: "test: ensure output is large enough to pay for its fees"
(https://github.com/bitcoin/bitcoin/pull/29283#pullrequestreview-1832846810)
utACK 3bfc5bd36e38ca07306a41a3f56ea102ffe02b67, didn't experience this issue but in theory a minimum of `AMOUNT_DUST` could be too low to pay the fees

💬 glozow commented on pull request "test: ensure output is large enough to pay for its fees":
(https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1459106218)
Adding `AMOUNT_DUST` isn't really necessary, it's just a floor

💬 jamesob commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1900564313)
> If that doesn't help, please benchmark libsecp256k1 with ([bitcoin-core/secp256k1@10e6d29](https://github.com/bitcoin-core/secp256k1/commit/10e6d29b60c3931e327bc18e6c50cea78296b1ba)) and without [bitcoin-core/secp256k1#1446](https://github.com/bitcoin-core/secp256k1/pull/1446) ([bitcoin-core/secp256k1@07687e8](https://github.com/bitcoin-core/secp256k1/commit/07687e811d1c9700e6fe9d658aef080e3568c0f1)), see the PR for instructions and also consider setting `SECP256K1_BENCH_ITERS=200000` or anoth
...

💬 glozow commented on issue "Enable `maxfeerate` and `maxburnamount` as startup config options.":
(https://github.com/bitcoin/bitcoin/issues/29217#issuecomment-1900585661)
fwiw I'm concept -0 on adding a global `maxburnamount` option. I think it's less footgunny for the default to always be 0 and for users specify explicitly each time they want to burn money. But if people really want this then ok.

🤔 furszy reviewed a pull request: "test: ensure output is large enough to pay for its fees"
(https://github.com/bitcoin/bitcoin/pull/29283#pullrequestreview-1832956095)
Can be replicated locally by changing line 273 to use the `get_rand_amount()` floor.
```diff
variant.initial_amount = AMOUNT_DUST * 2
```

🤔 glozow reviewed a pull request: "RPC: Wallet: Add `maxfeerate` and `maxburnamount` startup option"
(https://github.com/bitcoin/bitcoin/pull/29278#pullrequestreview-1832875650)
I mentioned adding a `maxfeerate` option in #29220, but I was more imagining it as a wallet-only option. I don't think that wallet configurations should bleed into how the node RPCs function. Instead, the interaction there should be for wallet to pass it as a param to `BroadcastTransaction` when the it submits a tx to the node.

I'm also not sure about `maxburnamount` being a node-wide config (https://github.com/bitcoin/bitcoin/issues/29217#issuecomment-1900585661)

💬 glozow commented on pull request "RPC: Wallet: Add `maxfeerate` and `maxburnamount` startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1459145558)
Why not just make this a `CFeeRate`? Or describe this as satoshis per KvB. "fee rate (in satoshis)" doesn't really make sense to me.

💬 glozow commented on pull request "RPC: Wallet: Add `maxfeerate` and `maxburnamount` startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1459122123)
I think you accidentally put the `maxburnamount` helptext in the `incrementalrelayfee` helptext here?

💬 glozow commented on pull request "RPC: Wallet: Add `maxfeerate` and `maxburnamount` startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1459131869)
Why delete these options (suddenly removing support for something that users might rely on)? Even if a config exists, I can imagine somebody wanting to change the maximum for a single RPC call without needing to restart their node.

💬 glozow commented on pull request "RPC: Wallet: Add `maxfeerate` and `maxburnamount` startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1459156381)
I also don't see this value being used anywhere other than `settxfee`?

> This approach maintains distinct role for maxfeerate as a wallet and mempool acceptance/relay sanity check

Shouldn't there be a check in `CreateTransactionInternal` that makes sure we don't make transactions above this feerate, and an arg passed to `BroadcastTransaction`? I also don't see a test case for it.

👍 ryanofsky approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1832865694)
Code review ACK 492ff2c504d1da3f4ae8704a5485a286ee76f0d9. This looks great, and thanks for taking so many suggestions. I did make a few more suggestions, but none are important so feel free to ignore them.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459145667)
In commit "refactor: De-globalize g_signals" (e7c060866267fb99c77f80a959bfeb5204d1d3a5)

Maybe move this declaration up next to the notifications member since both are ways of receiving notifications from validation code.

- `NodeContext::notifications` sends high level messages about sync status and errors and warnings, which are blocking and only received by a single recipient (the UI).
- `NodeContext::validation_signals` sends lower messages about blocks and transactions, which are non
...

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459116286)
In commit "refactor: De-globalize g_signals" (e7c060866267fb99c77f80a959bfeb5204d1d3a5)

I think it would be more consistent to use the local `validation_signals` variable here instead of the node variable. Same on line 1608 below