π¬ achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2164872311)
Done
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2164872311)
Done
π solveforceapp opened a pull request: "~"
(https://github.com/bitcoin/bitcoin/pull/32806)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/32806)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
β
fanquake closed a pull request: "~"
(https://github.com/bitcoin/bitcoin/pull/32806)
(https://github.com/bitcoin/bitcoin/pull/32806)
π¬ real-or-random commented on pull request "test: disable secp256 tests by default":
(https://github.com/bitcoin/bitcoin/pull/32782#issuecomment-3001843254)
> Alternatively, libsecp256k1 tests could be run with a lower value for the `SECP256K1_TEST_ITERS` environment variable: say, 8.
We reduced the default count to 16 last year. It would be interesting to know how long CI takes for a count of 8 or 4. And if that's not quite satisfying, we could disable some of the static (= not randomized) tests at low iteration counts. (We already do this for the most expensive ones.)
We test many many compile configurations (platform, compiler version, lib
...
(https://github.com/bitcoin/bitcoin/pull/32782#issuecomment-3001843254)
> Alternatively, libsecp256k1 tests could be run with a lower value for the `SECP256K1_TEST_ITERS` environment variable: say, 8.
We reduced the default count to 16 last year. It would be interesting to know how long CI takes for a count of 8 or 4. And if that's not quite satisfying, we could disable some of the static (= not randomized) tests at low iteration counts. (We already do this for the most expensive ones.)
We test many many compile configurations (platform, compiler version, lib
...
π€ ryanofsky reviewed a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2955388404)
Updated ad0144022c2bbed6799bafa3ac8b80ac81dd7178 -> 9ffe57f81b2a6abd161ae010f07916b05e57191d ([`pr/ipc-cli.7`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.7) -> [`pr/ipc-cli.8`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-cli.7..pr/ipc-cli.8)) with suggested `bitcoin-node` fix and documentation update.
---
re: https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2932651216
> Although wai
...
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2955388404)
Updated ad0144022c2bbed6799bafa3ac8b80ac81dd7178 -> 9ffe57f81b2a6abd161ae010f07916b05e57191d ([`pr/ipc-cli.7`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.7) -> [`pr/ipc-cli.8`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-cli.7..pr/ipc-cli.8)) with suggested `bitcoin-node` fix and documentation update.
---
re: https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2932651216
> Although wai
...
π¬ ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2164936628)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2150327388
> Could use a mention in src/interfaces/README.md
Thanks! Added
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2164936628)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2150327388
> Could use a mention in src/interfaces/README.md
Thanks! Added
π¬ ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2164936316)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2150571456
> unused?
IIUC, `std::string` does seem to be used in the Ipc::connectAddress and Ipc::listenAddress declarations below so keeping for now
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2164936316)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2150571456
> unused?
IIUC, `std::string` does seem to be used in the Ipc::connectAddress and Ipc::listenAddress declarations below so keeping for now
π¬ davidgumberg commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3001952939)
> The relevant code is in UDPRelayBlock
`UDPRelayBlock` is relevant only for FIBRE to FIBRE connections, and these also have totally bespoke logic for receiving FIBRE block announcements, which aren't the same as `CMPCTBLOCK` messages. The way that FIBRE nodes announce to "default" nodes is the same as the way that "default" nodes announce to each other. [Below](https://github.com/w0xlt/bitcoinfibre/blob/45897a826b37eb417cba93ac17ff7bebb272893c/src/net_processing.cpp#L881) the first snippet you
...
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3001952939)
> The relevant code is in UDPRelayBlock
`UDPRelayBlock` is relevant only for FIBRE to FIBRE connections, and these also have totally bespoke logic for receiving FIBRE block announcements, which aren't the same as `CMPCTBLOCK` messages. The way that FIBRE nodes announce to "default" nodes is the same as the way that "default" nodes announce to each other. [Below](https://github.com/w0xlt/bitcoinfibre/blob/45897a826b37eb417cba93ac17ff7bebb272893c/src/net_processing.cpp#L881) the first snippet you
...
π solveforceapp opened a pull request: "~"
(https://github.com/bitcoin/bitcoin/pull/32807)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/32807)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
β
fanquake closed a pull request: "~"
(https://github.com/bitcoin/bitcoin/pull/32807)
(https://github.com/bitcoin/bitcoin/pull/32807)
π¬ kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-3001984288)
> Missing line in unit and functional tests coverage.
A good follow-up could be to add coverage to this line in the functional tests
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-3001984288)
> Missing line in unit and functional tests coverage.
A good follow-up could be to add coverage to this line in the functional tests
π€ pablomartin4btc reviewed a pull request: "cli: rework -addrinfo cli to use addresses which arenβt filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-2955473819)
tACK c2f3225622cbbe516ac23059bff47ed3c28653cf
> my personal preference is for displaying only the relevant total addresses stats
I also lean toward showing only the total known addresses, as I think it better reflects what the node has in its addrman and avoids suggesting that only the filtered set "counts."
I'd consider whether passing an argument might offer one or both views (filtered/unfiltered) depending on the use case, but Iβve read the previous discussions here and in #27511, and
...
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-2955473819)
tACK c2f3225622cbbe516ac23059bff47ed3c28653cf
> my personal preference is for displaying only the relevant total addresses stats
I also lean toward showing only the total known addresses, as I think it better reflects what the node has in its addrman and avoids suggesting that only the filtered set "counts."
I'd consider whether passing an argument might offer one or both views (filtered/unfiltered) depending on the use case, but Iβve read the previous discussions here and in #27511, and
...
π¬ stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2164992810)
I agree `NeedsRateLimiting` has too many responsibilities. I think it's okay as-is, but your comment nerdsniped me into revisiting this. I'm working on a branch that removes the side effects from `NeedsRateLimiting` and a couple of other improvements. I'm not sure yet if it'll be too much of a detour for this PR, but at least it'll be interesting to be able to compare. Should have it ready tomorrow.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2164992810)
I agree `NeedsRateLimiting` has too many responsibilities. I think it's okay as-is, but your comment nerdsniped me into revisiting this. I'm working on a branch that removes the side effects from `NeedsRateLimiting` and a couple of other improvements. I'm not sure yet if it'll be too much of a detour for this PR, but at least it'll be interesting to be able to compare. Should have it ready tomorrow.
π¬ fjahr commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164987582)
Did you consider avoiding the dependency to node context and just give the Inteface it's own `CScheduler` instead?
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164987582)
Did you consider avoiding the dependency to node context and just give the Inteface it's own `CScheduler` instead?
π¬ fjahr commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164975420)
nit: new blank line seems unnecessary
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164975420)
nit: new blank line seems unnecessary
π€ fjahr reviewed a pull request: "rpc: use CScheduler for HTTPRPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2955445625)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2955445625)
Concept ACK
π¬ fjahr commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164968472)
Seems weird to mention libevent here as a point of reference since it's in the past after this change is applied. I would put this in the commit message and just say that it's not accurate enough here.
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2164968472)
Seems weird to mention libevent here as a point of reference since it's in the past after this change is applied. I would put this in the commit message and just say that it's not accurate enough here.
π€ w0xlt reviewed a pull request: "descriptors: Allow `H` as a hardened indicator"
(https://github.com/bitcoin/bitcoin/pull/32788#pullrequestreview-2955511313)
ACK https://github.com/bitcoin/bitcoin/pull/32788/commits/b6252655fe502ed1bbb97ab9597d9bbcf8c4484c
(https://github.com/bitcoin/bitcoin/pull/32788#pullrequestreview-2955511313)
ACK https://github.com/bitcoin/bitcoin/pull/32788/commits/b6252655fe502ed1bbb97ab9597d9bbcf8c4484c
π€ w0xlt reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2955519593)
reACK https://github.com/bitcoin/bitcoin/pull/31244/commits/5fe7915c865a8e7c0a95ec376d0f1ff737f5d1c2
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2955519593)
reACK https://github.com/bitcoin/bitcoin/pull/31244/commits/5fe7915c865a8e7c0a95ec376d0f1ff737f5d1c2
π¬ glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3002071721)
> Does it ever make sense to have multiple announcements for the same wtxid to be m_reconsider? I'm wondering if AddChildrenToWorkSet should skip wtxids for which a reconsider-announcement already exists.
Indeed, we don't need to reconsider the same transaction more than once if nothing's changed. I'm wondering how bad it is for `AddChildrenToWorkSet` to iterate through all of them every time - it might be slightly more efficient to allow multiple `m_reconsider`s and reset all to false in `Ge
...
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3002071721)
> Does it ever make sense to have multiple announcements for the same wtxid to be m_reconsider? I'm wondering if AddChildrenToWorkSet should skip wtxids for which a reconsider-announcement already exists.
Indeed, we don't need to reconsider the same transaction more than once if nothing's changed. I'm wondering how bad it is for `AddChildrenToWorkSet` to iterate through all of them every time - it might be slightly more efficient to allow multiple `m_reconsider`s and reset all to false in `Ge
...