⚠️ 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.
💬 ryanofsky commented on 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#discussion_r2501459500)
In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)
I think it's probably be good to just write the entire warning message here instead of just the beginning of the message, so it is clear what warning this code is trying to suppress.
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501459500)
In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)
I think it's probably be good to just write the entire warning message here instead of just the beginning of the message, so it is clear what warning this code is trying to suppress.
💬 ryanofsky commented on 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#discussion_r2501455784)
In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)
Could this change to test_framework.py be reverted?
I think it should be impossible for the warning to trigger here since the capnp module will already be imported by interface_ipc.py before this runs.
If changing this method is actually necessary, I think it'd be good to move common code importing capnp and suppressing the warning to a function in test_framework/util.py th
...
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501455784)
In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)
Could this change to test_framework.py be reverted?
I think it should be impossible for the warning to trigger here since the capnp module will already be imported by interface_ipc.py before this runs.
If changing this method is actually necessary, I think it'd be good to move common code importing capnp and suppressing the warning to a function in test_framework/util.py th
...
💬 ryanofsky commented on 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#discussion_r2501470819)
re: https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500471963
> I am thinking about setting `PYTHON_GIL=1` in the test runner, so that tests pick it up.
This seems like a reasonable alternative, but if we take this approach I hope we only do it selectively for the tests which need it.
I understand we may have some tests which aren't thread safe (though hopefully not too many if most tests are single-threaded) and it may be a lot easier to set `PYTHON_GIL=1` than to fix them.
...
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501470819)
re: https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500471963
> I am thinking about setting `PYTHON_GIL=1` in the test runner, so that tests pick it up.
This seems like a reasonable alternative, but if we take this approach I hope we only do it selectively for the tests which need it.
I understand we may have some tests which aren't thread safe (though hopefully not too many if most tests are single-threaded) and it may be a lot easier to set `PYTHON_GIL=1` than to fix them.
...
💬 0xB10C commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3500330037)
I load the `asmap.dat` file on by using `asmap=/absolute/path/to/asmap.dat` in my [config](https://github.com/0xB10C/peer-observer-infra-library/blob/e14fc1e29a3adbd35d5e3af73766039279fbf5c4/modules/node/node.nix#L225). Since this PR doesn't change anything for me, I'm neutral on this too.
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3500330037)
I load the `asmap.dat` file on by using `asmap=/absolute/path/to/asmap.dat` in my [config](https://github.com/0xB10C/peer-observer-infra-library/blob/e14fc1e29a3adbd35d5e3af73766039279fbf5c4/modules/node/node.nix#L225). Since this PR doesn't change anything for me, I'm neutral on this too.
⚠️ alejandrothugs9119 opened an issue: "lawl"
(https://github.com/bitcoin/bitcoin/issues/33812)
https://github.com/alejandrothugs9119/cryptoblock/tree/master
(https://github.com/bitcoin/bitcoin/issues/33812)
https://github.com/alejandrothugs9119/cryptoblock/tree/master
📝 Ataraxia009 opened a pull request: "Changing the rpcbind argument being ignored to a pop up warning, inst…"
(https://github.com/bitcoin/bitcoin/pull/33813)
When we ignore a users explicit request to an `rpcbind`, i think it warrants for a warning instead of a debug log.
(https://github.com/bitcoin/bitcoin/pull/33813)
When we ignore a users explicit request to an `rpcbind`, i think it warrants for a warning instead of a debug log.
💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#issuecomment-3500930821)
> I notice that CTransaction::ToString() prints the inputs and the input script witnesses additionally, with that change script witnesses will be included duplicated. The order there is first all inputs, then all witnesses. Please consider removing the duplication.
Hey, good call out. Updated to remove duplicate. Think this makes the most sense.
(https://github.com/bitcoin/bitcoin/pull/33711#issuecomment-3500930821)
> I notice that CTransaction::ToString() prints the inputs and the input script witnesses additionally, with that change script witnesses will be included duplicated. The order there is first all inputs, then all witnesses. Please consider removing the duplication.
Hey, good call out. Updated to remove duplicate. Think this makes the most sense.