💬 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

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

I see 2 changes to this function which are necessary, and 9 changes which are not needed. It's not important, but I'd suggest reverting all the changes in this function except the on lines 1145-6 above and lines 1167-8 below. Moving the `std::make_unique<CScheduler>()` call is the most important change here, and the other changes do not seem like a significant cleanup and are distracting.

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

Possible inconsistency between signal/signals here

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

Maybe some unintended whitespace changes here and below

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459185693)
In commit "kernel: Remove dependency on CScheduler" (492ff2c504d1da3f4ae8704a5485a286ee76f0d9)

I think it would be clearer just to inline these methods and eliminate this file. If not eliminating this file, I would also consider it renaming it to just `util/task_runner.cpp` so it easier to find next to the header file.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459197068)
In commit "kernel: Remove dependency on CScheduler" (492ff2c504d1da3f4ae8704a5485a286ee76f0d9)

I'd consider the leaving this declaration inside the ValidationSignalsImpl class instead of moving it here. This would adhere more closely to the [pointer-to-implementation](https://en.cppreference.com/w/cpp/language/pimpl) pattern and ensure future ValidationSignalsImpl methods can access all private members of the class, not just some of them. It would also just move less code around unnecessarily
...