💬 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
💬 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_r1459253396)
We use subtraction, and should try to handle that?
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253396)
We use subtraction, and should try to handle that?
💬 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_r1459253460)
done
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253460)
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_r1459253539)
added comment
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253539)
added comment
💬 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_r1459253631)
done
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253631)
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_r1459253686)
done
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253686)
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_r1459253728)
done
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459253728)
done
👍 murchandamus approved a pull request: "wallet: fix coin selection tracing to return -1 when no change pos"
(https://github.com/bitcoin/bitcoin/pull/29272#pullrequestreview-1833088422)
tACK with nit: e701982c741edd2182690ddf4628e4c079a5185e
(https://github.com/bitcoin/bitcoin/pull/29272#pullrequestreview-1833088422)
tACK with nit: e701982c741edd2182690ddf4628e4c079a5185e
💬 murchandamus commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1459228682)
There were a couple transactions before, each sending 10 BTC to the wallet itself. So the wallet should still have a balance of 50 – fees BTC, and presumably 3 UTXOs of 10 BTC, 10 BTC, and 30 - fees BTC. I‘m not sure why APS (avoid partial spend) would make a difference here. APS just means that we’ll force use of all coins associated with the same scriptpubkey at once rather than a single UTXO if there is address reuse, but it seems to me that funds are always sent to new addresses here.
It
...
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1459228682)
There were a couple transactions before, each sending 10 BTC to the wallet itself. So the wallet should still have a balance of 50 – fees BTC, and presumably 3 UTXOs of 10 BTC, 10 BTC, and 30 - fees BTC. I‘m not sure why APS (avoid partial spend) would make a difference here. APS just means that we’ll force use of all coins associated with the same scriptpubkey at once rather than a single UTXO if there is address reuse, but it seems to me that funds are always sent to new addresses here.
It
...