🤔 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)
(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.
(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?
(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.
(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.
(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.
(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
...
(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
(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.
(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
(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
(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.
(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
...
(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
...
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459176013)
In commit "scripted-diff: Rename SingleThreadedSchedulerClient to SerialTaskRunner" (46defde7a60afe186629481b738225919f874342)
I think I made a bad suggestion earlier to call this method "clear". I mistakenly thought this was discarding callbacks, but it appears to wait for all the callbacks to run. In light of this, I think it would be better to call this method "flush" or "wait".
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459176013)
In commit "scripted-diff: Rename SingleThreadedSchedulerClient to SerialTaskRunner" (46defde7a60afe186629481b738225919f874342)
I think I made a bad suggestion earlier to call this method "clear". I mistakenly thought this was discarding callbacks, but it appears to wait for all the callbacks to run. In light of this, I think it would be better to call this method "flush" or "wait".
💬 sipa commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1900637798)
Wow, bizarre, but not impossible.
It seems that the handwritten asm code we had was faster on your CPU, but is slower on all CPUs we tested on.
Yours seems to be a fairly old architecture (2nd gen Intel Core), so it's not impossible there are differences.
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1900637798)
Wow, bizarre, but not impossible.
It seems that the handwritten asm code we had was faster on your CPU, but is slower on all CPUs we tested on.
Yours seems to be a fairly old architecture (2nd gen Intel Core), so it's not impossible there are differences.
💬 mzumsande commented on issue "Post startup stalling":
(https://github.com/bitcoin/bitcoin/issues/29281#issuecomment-1900665438)
Another question is how to deal with it after having a separate detection criterion.
The stalling logic is designed such that if we are in a situation where other peers are blocked by the staller, the staller will be disconnected (hopefully downloading the missing block from someone else and unblocking the other peers with unused download slots). In the situation here, there is probably no other peer blocked; we just want to get to the tip faster, rather than waiting for 10+ minutes (`BLOCK_DOW
...
(https://github.com/bitcoin/bitcoin/issues/29281#issuecomment-1900665438)
Another question is how to deal with it after having a separate detection criterion.
The stalling logic is designed such that if we are in a situation where other peers are blocked by the staller, the staller will be disconnected (hopefully downloading the missing block from someone else and unblocking the other peers with unused download slots). In the situation here, there is probably no other peer blocked; we just want to get to the tip faster, rather than waiting for 10+ minutes (`BLOCK_DOW
...
💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459252934)
done
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459252934)
done
💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253057)
took some of hte language, and explicitly defined each term
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253057)
took some of hte language, and explicitly defined each term
💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253223)
done
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253223)
done
💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253328)
done
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253328)
done