✅ fanquake closed an issue: "In the future "
(https://github.com/bitcoin/bitcoin/issues/29168)
  (https://github.com/bitcoin/bitcoin/issues/29168)
:lock: fanquake locked an issue: "In the future "
(https://github.com/bitcoin/bitcoin/issues/29168)
  (https://github.com/bitcoin/bitcoin/issues/29168)
👍 dergoegge approved a pull request: "mempool: Don't sort in entryAll"
(https://github.com/bitcoin/bitcoin/pull/29019#pullrequestreview-1802291377)
utACK f761f0306b092c06cecd239060b36cd0ba45fa2c
  (https://github.com/bitcoin/bitcoin/pull/29019#pullrequestreview-1802291377)
utACK f761f0306b092c06cecd239060b36cd0ba45fa2c
👍 furszy approved a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1802295539)
reACK b093e5e
Only cleaned the IsMine && IsFromMe code dup from my last review.
  (https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1802295539)
reACK b093e5e
Only cleaned the IsMine && IsFromMe code dup from my last review.
✅ fanquake closed an issue: "Possibility to dump all runtime parameter values"
(https://github.com/bitcoin/bitcoin/issues/28790)
  (https://github.com/bitcoin/bitcoin/issues/28790)
💬 fanquake commented on issue "Possibility to dump all runtime parameter values":
(https://github.com/bitcoin/bitcoin/issues/28790#issuecomment-1875492104)
Going to close this for now.
  (https://github.com/bitcoin/bitcoin/issues/28790#issuecomment-1875492104)
Going to close this for now.
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1875530051)
Rebased 717093f24b1ad65bc18b23eab492cfdcc2511e69 -> d63b2e88780dc78fd531b053653361a0bf3fcbea ([noGlobalSignals_0](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_0) -> [noGlobalSignals_1](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_0..noGlobalSignals_1))
* Fix conflict with #29013
  (https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1875530051)
Rebased 717093f24b1ad65bc18b23eab492cfdcc2511e69 -> d63b2e88780dc78fd531b053653361a0bf3fcbea ([noGlobalSignals_0](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_0) -> [noGlobalSignals_1](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_0..noGlobalSignals_1))
* Fix conflict with #29013
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1440574231)
> To me `net_processing.h` looks like the natural place to move/expose the constant(s) that were defined as `static` in `net_processing.cpp` if they need to be used outside of that `cpp` file. Do you somehow dislike that idea or have another one?
I don't totally dislike it, but was pondering the option of moving them to a separate file for two reasons; I find the spread of constants across the sources hard to maintain, and I was also thinking about another testing scenario: what if we want to
...
  (https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1440574231)
> To me `net_processing.h` looks like the natural place to move/expose the constant(s) that were defined as `static` in `net_processing.cpp` if they need to be used outside of that `cpp` file. Do you somehow dislike that idea or have another one?
I don't totally dislike it, but was pondering the option of moving them to a separate file for two reasons; I find the spread of constants across the sources hard to maintain, and I was also thinking about another testing scenario: what if we want to
...
🤔 glozow reviewed a pull request: "mempool: Don't sort in entryAll"
(https://github.com/bitcoin/bitcoin/pull/29019#pullrequestreview-1802420294)
ACK f761f0306b092c06cecd239060b36cd0ba45fa2c given this brings us back to what we had before
It seems correct that none of the callsites need `GetSortedDepthAndScore`, though maybe topological is needed in wallet? I had the [same](https://github.com/bitcoin/bitcoin/pull/29019#issuecomment-1845497136) question. I guess insertion order doesn't imply topological if a parent was inserted later than a child, i.e. if it's from a reorg?
  (https://github.com/bitcoin/bitcoin/pull/29019#pullrequestreview-1802420294)
ACK f761f0306b092c06cecd239060b36cd0ba45fa2c given this brings us back to what we had before
It seems correct that none of the callsites need `GetSortedDepthAndScore`, though maybe topological is needed in wallet? I had the [same](https://github.com/bitcoin/bitcoin/pull/29019#issuecomment-1845497136) question. I guess insertion order doesn't imply topological if a parent was inserted later than a child, i.e. if it's from a reorg?
💬 mzumsande commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1440584344)
Just a code comment would be fine too. I agree that the rounding doesn't matter at all in this context (and it'd also be fine not to calculate the mean in the even case at all), but code tends to get copied and reused.
  (https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1440584344)
Just a code comment would be fine too. I agree that the rounding doesn't matter at all in this context (and it'd also be fine not to calculate the mean in the even case at all), but code tends to get copied and reused.
💬 TheCharlatan commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1875562921)
Concept ACK
  (https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1875562921)
Concept ACK
👍 dergoegge approved a pull request: "fuzz: rule-out too deep derivation paths in descriptor parsing targets"
(https://github.com/bitcoin/bitcoin/pull/28832#pullrequestreview-1802469073)
ACK a44808fb437864878c2d9696b8a96193091446ee - Not running into timeouts anymore
https://dergoegge.github.io/bitcoin-coverage/pr28832/fuzz.coverage/index.html
  (https://github.com/bitcoin/bitcoin/pull/28832#pullrequestreview-1802469073)
ACK a44808fb437864878c2d9696b8a96193091446ee - Not running into timeouts anymore
https://dergoegge.github.io/bitcoin-coverage/pr28832/fuzz.coverage/index.html
💬 dergoegge commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1875574282)
Coverage for `coins_db`: https://dergoegge.github.io/bitcoin-coverage/pr28216/fuzz.coverage/index.html
  (https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1875574282)
Coverage for `coins_db`: https://dergoegge.github.io/bitcoin-coverage/pr28216/fuzz.coverage/index.html
💬 dergoegge commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1875575611)
Coverage for `block_index`: https://dergoegge.github.io/bitcoin-coverage/pr28209/fuzz.coverage/index.html
  (https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1875575611)
Coverage for `block_index`: https://dergoegge.github.io/bitcoin-coverage/pr28209/fuzz.coverage/index.html
📝 fanquake opened a pull request: "Update libsecp256k1 subtree for 0.4.1 release"
(https://github.com/bitcoin/bitcoin/pull/29169)
Updates our libsecp256k1 subtree for the 0.4.1 release: https://github.com/bitcoin-core/secp256k1/releases/tag/v0.4.1.
> The point multiplication algorithm used for ECDH operations (module ecdh) was replaced with a slightly faster one.
> Optional handwritten x86_64 assembly for field operations was removed because modern C compilers are able to output more efficient assembly. This change results in a significant speedup of some library functions when handwritten x86_64 assembly is enabled
...
  (https://github.com/bitcoin/bitcoin/pull/29169)
Updates our libsecp256k1 subtree for the 0.4.1 release: https://github.com/bitcoin-core/secp256k1/releases/tag/v0.4.1.
> The point multiplication algorithm used for ECDH operations (module ecdh) was replaced with a slightly faster one.
> Optional handwritten x86_64 assembly for field operations was removed because modern C compilers are able to output more efficient assembly. This change results in a significant speedup of some library functions when handwritten x86_64 assembly is enabled
...
💬 sipa commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1875614423)
Obvious ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 is obvious.
  (https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1875614423)
Obvious ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 is obvious.
💬 fanquake commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1875616633)
cc @real-or-random @jonasnick
  (https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1875616633)
cc @real-or-random @jonasnick
🤔 sr-gi reviewed a pull request: "Nuke adjusted time (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1800831281)
Concept ACK
  (https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1800831281)
Concept ACK
💬 sr-gi commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1439751712)
Out of curiosity: are you grouping these two so you can query them together, given you'll need both of them in `getnetworkinfo` if peerman is defined?
  (https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1439751712)
Out of curiosity: are you grouping these two so you can query them together, given you'll need both of them in `getnetworkinfo` if peerman is defined?
💬 sr-gi commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1439717289)
In order to be consistent with how the code used to be what needs updating is the code and not the comment, doesn't it? Otherwise, we will be computing the median as low as with 4 elements when we used to need 5 before.
I don't know what the motivation to require 5 data points previously was (why 4 or 3?), but my impression by reading the comments in this PR is that we are trying no to change anyting with respect to how the median is computed and leave that for a followup
  (https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1439717289)
In order to be consistent with how the code used to be what needs updating is the code and not the comment, doesn't it? Otherwise, we will be computing the median as low as with 4 elements when we used to need 5 before.
I don't know what the motivation to require 5 data points previously was (why 4 or 3?), but my impression by reading the comments in this PR is that we are trying no to change anyting with respect to how the median is computed and leave that for a followup