💬 ajtowns commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3499059367)
> Doe this mean that, due to `AreInputsStandard`, even more UTXOs were soft-confiscated when #12460 was added?
I think that's pretty likely, but haven't confirmed. Should mean that bare multisigs are only spendable if any data is pushed as a fake un/compressed pubkey.
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3499059367)
> Doe this mean that, due to `AreInputsStandard`, even more UTXOs were soft-confiscated when #12460 was added?
I think that's pretty likely, but haven't confirmed. Should mean that bare multisigs are only spendable if any data is pushed as a fake un/compressed pubkey.
👍 hodlinator approved a pull request: "util: Allow `Assert` (et al.) in contexts without __func__"
(https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3430025702)
re-ACK fad6efd3bef1d123f806d492f019e29530b03a5e
2 added commits since previous review (https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3427192513):
* Remove no longer needed disabling of linter according to CI. 👍 (Re-touching `NONFATAL_UNREACHABLE` but no big deal).
* De-duplicate code by using `STR_INTERNAL_BUG()`.
(https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3430025702)
re-ACK fad6efd3bef1d123f806d492f019e29530b03a5e
2 added commits since previous review (https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3427192513):
* Remove no longer needed disabling of linter according to CI. 👍 (Re-touching `NONFATAL_UNREACHABLE` but no big deal).
* De-duplicate code by using `STR_INTERNAL_BUG()`.
💬 hodlinator commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2500429489)
nit: This is a case of clang-format failing to understand what is happening inside a macro.
https://releases.llvm.org/17.0.1/tools/clang/docs/ClangFormatStyleOptions.html#cpp11bracedliststyle
https://github.com/bitcoin/bitcoin/blob/fada379589a17e86396aa7c2ce458ff2ff602b84/src/.clang-format#L87
If I in the same commit change this to...
```C++
throw NonFatalCheckError{"Unreachable code reached (non-fatal)", std::source_location::current()}
```
...and also add...
```C++
void Func_NONFATAL_
...
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2500429489)
nit: This is a case of clang-format failing to understand what is happening inside a macro.
https://releases.llvm.org/17.0.1/tools/clang/docs/ClangFormatStyleOptions.html#cpp11bracedliststyle
https://github.com/bitcoin/bitcoin/blob/fada379589a17e86396aa7c2ce458ff2ff602b84/src/.clang-format#L87
If I in the same commit change this to...
```C++
throw NonFatalCheckError{"Unreachable code reached (non-fatal)", std::source_location::current()}
```
...and also add...
```C++
void Func_NONFATAL_
...
💬 flack commented on pull request "refactor: remove dead branches in `SingletonClusterImpl`":
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500676630)
this now always returns true. Is that needed for anything?
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500676630)
this now always returns true. Is that needed for anything?
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500696484)
We discussed this out of band, here's the summary:
* the main thread is special because it has access to the dbcache, it can insert there without locking the cache.
* the threads each have their inputs now, each of which have a switch to signal when they've fetched the input and move on to the next
* the threads each compete for which input to fetch, marking them one-by-one as ready
* the main thread goes in order, spins until the next one is available, if it needs to wait, it does some fetc
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500696484)
We discussed this out of band, here's the summary:
* the main thread is special because it has access to the dbcache, it can insert there without locking the cache.
* the threads each have their inputs now, each of which have a switch to signal when they've fetched the input and move on to the next
* the threads each compete for which input to fetch, marking them one-by-one as ready
* the main thread goes in order, spins until the next one is available, if it needs to wait, it does some fetc
...
💬 l0rinc commented on pull request "refactor: remove dead branches in `SingletonClusterImpl`":
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500745964)
Good question, I have already checked that, the sibling [GenericClusterImpl::Split](https://github.com/bitcoin/bitcoin/blob/master/src/txgraph.cpp#L1392) can return `false` which would [trigger deletion](https://github.com/bitcoin/bitcoin/blob/master/src/txgraph.cpp#L1773-L1777).
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500745964)
Good question, I have already checked that, the sibling [GenericClusterImpl::Split](https://github.com/bitcoin/bitcoin/blob/master/src/txgraph.cpp#L1392) can return `false` which would [trigger deletion](https://github.com/bitcoin/bitcoin/blob/master/src/txgraph.cpp#L1773-L1777).
💬 sipa commented on pull request "refactor: remove dead branches in `SingletonClusterImpl`":
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500769009)
Yeah. It's indeed the case that within `SingletonClusterImpl` it'll always return `true`, but this isn't the case for all possible `Cluster` implementations.
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500769009)
Yeah. It's indeed the case that within `SingletonClusterImpl` it'll always return `true`, but this isn't the case for all possible `Cluster` implementations.
⚠️ valentinewallace opened an issue: "REST equivalent for `gettxspendingprevout`"
(https://github.com/bitcoin/bitcoin/issues/33808)
This becomes more useful with #24539.
This would be useful for Lightning nodes so that they can use Bitcoin Core in situations where they currently use Electrum.
For example, an LSP has many mobile users who all connect to the LSP's Electrum instance to get information on whether their funding/commitment transactions have confirmed, etc.
Instead, they could be using the Core REST interface to retrieve this info, using `gettxspendingprevout` + @sstone’s PR.
A caveat is that resolving this
...
(https://github.com/bitcoin/bitcoin/issues/33808)
This becomes more useful with #24539.
This would be useful for Lightning nodes so that they can use Bitcoin Core in situations where they currently use Electrum.
For example, an LSP has many mobile users who all connect to the LSP's Electrum instance to get information on whether their funding/commitment transactions have confirmed, etc.
Instead, they could be using the Core REST interface to retrieve this info, using `gettxspendingprevout` + @sstone’s PR.
A caveat is that resolving this
...
💬 valentinewallace commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3499495731)
@sstone Question -- In LDK, when we confirm transactions, we require the transaction's index-in-block to be provided, for short channel ID construction. I'm curious how this API works for you without this information?
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3499495731)
@sstone Question -- In LDK, when we confirm transactions, we require the transaction's index-in-block to be provided, for short channel ID construction. I'm curious how this API works for you without this information?
⚠️ TheBlueMatt opened an issue: "Cache headers in REST"
(https://github.com/bitcoin/bitcoin/issues/33809)
it would be nice to serve appropriate cache headers in the REST interface so that you can just slap nginx/cloudflare in front of it and have a nice caching proxy. Eg getXbyheight requests would get cached for 5 seconds and getYbyhash requests could get cached for a month/year.
(https://github.com/bitcoin/bitcoin/issues/33809)
it would be nice to serve appropriate cache headers in the REST interface so that you can just slap nginx/cloudflare in front of it and have a nice caching proxy. Eg getXbyheight requests would get cached for 5 seconds and getYbyhash requests could get cached for a month/year.
💬 TheBlueMatt commented on issue "Cache headers in REST":
(https://github.com/bitcoin/bitcoin/issues/33809#issuecomment-3499514023)
@achow101 said she'd work on this
(https://github.com/bitcoin/bitcoin/issues/33809#issuecomment-3499514023)
@achow101 said she'd work on this
📝 hebasto opened a pull request: "ci: Add fast IWYU job"
(https://github.com/bitcoin/bitcoin/pull/33810)
This PR seeks to address feedback raised in the discussion of https://github.com/bitcoin/bitcoin/pull/33779, specifically from this [comment](https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491515263):
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
The new "iwyu" CI job runs very quickly
...
(https://github.com/bitcoin/bitcoin/pull/33810)
This PR seeks to address feedback raised in the discussion of https://github.com/bitcoin/bitcoin/pull/33779, specifically from this [comment](https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491515263):
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
The new "iwyu" CI job runs very quickly
...
💬 hebasto commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3499831767)
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
Done in https://github.com/bitcoin/bitcoin/pull/33810.
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3499831767)
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
Done in https://github.com/bitcoin/bitcoin/pull/33810.
💬 marcofleon commented on issue "RFC: Add multiprocess fuzz target":
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3499863564)
I'd say fuzzamoto is well suited for this. I added a proof-of-concept scenario testing the mining interface, which just randomly calls the methods from the `Mining` and `BlockTemplate` interfaces for now. I'm sure we could come up with more sophisticated scenarios that involve the `Wallet` and `Chain` interfaces as well. The scenario includes a mining IPC client that connects to the `bitcoin-node` Unix socket. It's currently directly in the scenario file but could be extracted into a reusable mu
...
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3499863564)
I'd say fuzzamoto is well suited for this. I added a proof-of-concept scenario testing the mining interface, which just randomly calls the methods from the `Mining` and `BlockTemplate` interfaces for now. I'm sure we could come up with more sophisticated scenarios that involve the `Wallet` and `Chain` interfaces as well. The scenario includes a mining IPC client that connects to the `bitcoin-node` Unix socket. It's currently directly in the scenario file but could be extracted into a reusable mu
...
🤔 w0xlt reviewed a pull request: "ci: Add fast IWYU job"
(https://github.com/bitcoin/bitcoin/pull/33810#pullrequestreview-3431033031)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33810#pullrequestreview-3431033031)
Concept ACK
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3499901560)
> it looks like in AcceptSingleTransaction() we acquire the lock and release it before the MemPoolAccept object is destroyed, causing a change to txgraph (when the changeset is destroyed) while the lock is not held. Will fix.
> Implemented a fix for the locking issue in https://github.com/bitcoin/bitcoin/commit/51d213d1449b61665628ce9b4e0209cff3c4f818.
IIUC we need to ensure we don’t release the mempool lock until after the changeset (i.e. SubPackageState) is destroyed.
The approach in
...
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3499901560)
> it looks like in AcceptSingleTransaction() we acquire the lock and release it before the MemPoolAccept object is destroyed, causing a change to txgraph (when the changeset is destroyed) while the lock is not held. Will fix.
> Implemented a fix for the locking issue in https://github.com/bitcoin/bitcoin/commit/51d213d1449b61665628ce9b4e0209cff3c4f818.
IIUC we need to ensure we don’t release the mempool lock until after the changeset (i.e. SubPackageState) is destroyed.
The approach in
...
📝 OGbencox opened a pull request: "Readme change"
(https://github.com/bitcoin/bitcoin/pull/33811)
<!--
*** 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/33811)
<!--
*** 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
...
💬 w0xlt commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2501363004)
Perhaps a debug-level log here indicating poor quality addresses ?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2501363004)
Perhaps a debug-level log here indicating poor quality addresses ?
💬 ryanofsky commented on issue "RFC: Add multiprocess fuzz target":
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3500144388)
Thanks! This looks really great and I am already dreading the crashes it may uncover, especially since I didn't fix the [first one](https://github.com/bitcoin/bitcoin/issues/33341#issuecomment-3266653805) yet.
I was looking at what I think is the main implementation file: [ipc_mining.rs](https://github.com/marcofleon/fuzzamoto/blob/ipc-mining/fuzzamoto-scenarios/bin/ipc_mining.rs) and it appears pretty neat and fairly easy to extend and maintain.
I am wondering what next steps may be needed to
...
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3500144388)
Thanks! This looks really great and I am already dreading the crashes it may uncover, especially since I didn't fix the [first one](https://github.com/bitcoin/bitcoin/issues/33341#issuecomment-3266653805) yet.
I was looking at what I think is the main implementation file: [ipc_mining.rs](https://github.com/marcofleon/fuzzamoto/blob/ipc-mining/fuzzamoto-scenarios/bin/ipc_mining.rs) and it appears pretty neat and fairly easy to extend and maintain.
I am wondering what next steps may be needed to
...
🤔 ryanofsky reviewed a pull request: "test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set"
(https://github.com/bitcoin/bitcoin/pull/33795#pullrequestreview-3431290651)
Code review 136dfde1c2284e30ecb1dfb35520f93878ec7335. I think this is a good approach, and that suppressing the warning is the most direct fix for the test failure that avoids reducing test coverage. I left some suggestions below for avoiding code duplication and documenting the workaround better. Also think the PR title and description could be updated since the test is no longer skipped.
(https://github.com/bitcoin/bitcoin/pull/33795#pullrequestreview-3431290651)
Code review 136dfde1c2284e30ecb1dfb35520f93878ec7335. I think this is a good approach, and that suppressing the warning is the most direct fix for the test failure that avoids reducing test coverage. I left some suggestions below for avoiding code duplication and documenting the workaround better. Also think the PR title and description could be updated since the test is no longer skipped.