📝 mzumsande opened a pull request: "validation: invalid block handling followups"
(https://github.com/bitcoin/bitcoin/pull/32843)
Some follow-ups from #31405:
- avoid duplicate recalculation of failure flags and `m_best_header` in `InvalidateBlock()`, which already does the accounting for these while disconnecting blocks, so recalcuation is not needed in the final `InvalidChainFound` call (https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2138368309)
- make it clearer that all block indexes with a `CBlockIndexWorkComparator` score at least as good as the tip (and no others) are expected in `setBlockIndexCandi
...
(https://github.com/bitcoin/bitcoin/pull/32843)
Some follow-ups from #31405:
- avoid duplicate recalculation of failure flags and `m_best_header` in `InvalidateBlock()`, which already does the accounting for these while disconnecting blocks, so recalcuation is not needed in the final `InvalidChainFound` call (https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2138368309)
- make it clearer that all block indexes with a `CBlockIndexWorkComparator` score at least as good as the tip (and no others) are expected in `setBlockIndexCandi
...
👍 theStack approved a pull request: "test: Fix wait_for_getheaders() call in test_outbound_eviction_blocks_relay_only()"
(https://github.com/bitcoin/bitcoin/pull/32823#pullrequestreview-2972945567)
lgtm ACK ec004cdb86e6471915e1033f390c76ee0428e415 :hash:
The second commit is a test bugfix very similar to #32742 / dd8447f70faf6419b4617da3c1b57098e9cd66a6 and should be backported (fwiw, I checked all other `wait_for_...` calls with hash parameters in this test file and think we caught them all now). The other commit improves the code comments w.r.t. consistent peer/node naming and getting rid of the "we" phrasing, agree that this is clearer now.
(https://github.com/bitcoin/bitcoin/pull/32823#pullrequestreview-2972945567)
lgtm ACK ec004cdb86e6471915e1033f390c76ee0428e415 :hash:
The second commit is a test bugfix very similar to #32742 / dd8447f70faf6419b4617da3c1b57098e9cd66a6 and should be backported (fwiw, I checked all other `wait_for_...` calls with hash parameters in this test file and think we caught them all now). The other commit improves the code comments w.r.t. consistent peer/node naming and getting rid of the "we" phrasing, agree that this is clearer now.
📝 luke-jr opened a pull request: "RPC/txoutproof: Support including (and verifying) proofs of wtxid"
(https://github.com/bitcoin/bitcoin/pull/32844)
Feature requested by @SomberNight for Electrum's Lightning stuff. (Please confirm this does what you need)
Unfortunately WIP because the wtxid merkle root calculation doesn't match and I don't have time to figure out why not yet (maybe after concept ACK? or if anyone else wants to take a look...)
Mostly backward compatible, but a segwit-enabled proof will verify the generation tx in old versions.
(https://github.com/bitcoin/bitcoin/pull/32844)
Feature requested by @SomberNight for Electrum's Lightning stuff. (Please confirm this does what you need)
Unfortunately WIP because the wtxid merkle root calculation doesn't match and I don't have time to figure out why not yet (maybe after concept ACK? or if anyone else wants to take a look...)
Mostly backward compatible, but a segwit-enabled proof will verify the generation tx in old versions.
📝 pablomartin4btc opened a pull request: "rpc, test: Fix JSON parsing errors in RPC calls (unloadwallet and getdescriptoractivity) raising RPC_INVALID_PARAMETER with appropriate description"
(https://github.com/bitcoin/bitcoin/pull/32845)
Currently, `unloadwallet` RPC call fails with a JSON parsing error when no `wallet_name` argument is provided. This behavior is misleading because the error originates from a low-level JSON type mismatch, rather than clearly indicating that the wallet name or RPC endpoint (`-rpcwallet=...`) is missing. Also, found out that the [issue](https://github.com/bitcoin/bitcoin/pull/13111#issuecomment-398831543) was noticed during its implementation but never addressed.
In addition, I've verified all
...
(https://github.com/bitcoin/bitcoin/pull/32845)
Currently, `unloadwallet` RPC call fails with a JSON parsing error when no `wallet_name` argument is provided. This behavior is misleading because the error originates from a low-level JSON type mismatch, rather than clearly indicating that the wallet name or RPC endpoint (`-rpcwallet=...`) is missing. Also, found out that the [issue](https://github.com/bitcoin/bitcoin/pull/13111#issuecomment-398831543) was noticed during its implementation but never addressed.
In addition, I've verified all
...
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2176170471)
Fixed, thanks.
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2176170471)
Fixed, thanks.
💬 SomberNight commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2176171672)
Is the cause of the mismatch that you thought the scriptPubKey directly contains the wtxid tree root?
It actually contains `Double-SHA256(witness root hash|witness reserved value)`
So you also have to extract the `witness reserved value` from the coinbase input witness.
see https://github.com/bitcoin/bips/blob/77b0bb297ad4e4e6b04b2c078c5e3b620a7090e2/bip-0141.mediawiki#commitment-structure
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2176171672)
Is the cause of the mismatch that you thought the scriptPubKey directly contains the wtxid tree root?
It actually contains `Double-SHA256(witness root hash|witness reserved value)`
So you also have to extract the `witness reserved value` from the coinbase input witness.
see https://github.com/bitcoin/bips/blob/77b0bb297ad4e4e6b04b2c078c5e3b620a7090e2/bip-0141.mediawiki#commitment-structure
💬 SomberNight commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021260691)
> Feature requested by @SomberNight for Electrum's Lightning stuff. (Please confirm this does what you need)
Wow, thank you for working on this! I was meaning to open an issue to ask for this, but did not even get to that yet.
The request I would like an electrum server, assuming txindex=1, to be able to serve is:
`bc.tx.get_merkle_witness(txid)`, (so given just a `txid`,)
return a dict containing:
- `wtxid`
- `block_height`
- `block_hash`
- `pos` (index of tx in block)
- `mer
...
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021260691)
> Feature requested by @SomberNight for Electrum's Lightning stuff. (Please confirm this does what you need)
Wow, thank you for working on this! I was meaning to open an issue to ask for this, but did not even get to that yet.
The request I would like an electrum server, assuming txindex=1, to be able to serve is:
`bc.tx.get_merkle_witness(txid)`, (so given just a `txid`,)
return a dict containing:
- `wtxid`
- `block_height`
- `block_hash`
- `pos` (index of tx in block)
- `mer
...
💬 SomberNight commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2176194870)
My cpp is beyond rusty, so maybe I misunderstand, but note that just because the tx to be proved does not have a witness, we would still want a witness-SPV proof for that tx.
The attack vector is that a malicious electrum server can notify the client about a tx touching their address and strip away the witness. That is, the actual tx has a witness, but the server sends it to the client using the old serialization format without it. Then when the client asks for the SPV proof, the server (stil
...
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2176194870)
My cpp is beyond rusty, so maybe I misunderstand, but note that just because the tx to be proved does not have a witness, we would still want a witness-SPV proof for that tx.
The attack vector is that a malicious electrum server can notify the client about a tx touching their address and strip away the witness. That is, the actual tx has a witness, but the server sends it to the client using the old serialization format without it. Then when the client asks for the SPV proof, the server (stil
...
💬 bigshiny90 commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2176203977)
so, something like this?
`// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
return {};
}
const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitness.stack[0];
if (witness_nonce.size() != 32) {
return {};
}
// Calculate the witness
...
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2176203977)
so, something like this?
`// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
return {};
}
const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitness.stack[0];
if (witness_nonce.size() != 32) {
return {};
}
// Calculate the witness
...
💬 bigshiny90 commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021345326)
@ghost43 hmm.. like this?
Here's the fix needed in `src/rpc/txoutproof.cpp` in the `verifytxoutproof` function after line 220:
```cpp
const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
return {};
}
const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitness.stack[0];
if (witness_nonce.size() !=
...
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021345326)
@ghost43 hmm.. like this?
Here's the fix needed in `src/rpc/txoutproof.cpp` in the `verifytxoutproof` function after line 220:
```cpp
const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
return {};
}
const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitness.stack[0];
if (witness_nonce.size() !=
...
💬 bigshiny90 commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021350135)
@SomberNight hmm.. like this?
Here's the fix needed in `src/rpc/txoutproof.cpp` in the `verifytxoutproof` function after line 220:
```cpp
const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
return {};
}
const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitness.stack[0];
if (witness_nonce.size(
...
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021350135)
@SomberNight hmm.. like this?
Here's the fix needed in `src/rpc/txoutproof.cpp` in the `verifytxoutproof` function after line 220:
```cpp
const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
return {};
}
const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitness.stack[0];
if (witness_nonce.size(
...
💬 bigshiny90 commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021358317)
i'm fairly new to this... let's see if that's correct.
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021358317)
i'm fairly new to this... let's see if that's correct.
💬 achow101 commented on pull request "doc: Add fetching single PRs from upstream to productivity.md":
(https://github.com/bitcoin/bitcoin/pull/32783#issuecomment-3021366309)
ACK 45b1d39757668939b03b27401c324a938ef0cd8d
(https://github.com/bitcoin/bitcoin/pull/32783#issuecomment-3021366309)
ACK 45b1d39757668939b03b27401c324a938ef0cd8d
🚀 achow101 merged a pull request: "doc: Add fetching single PRs from upstream to productivity.md"
(https://github.com/bitcoin/bitcoin/pull/32783)
(https://github.com/bitcoin/bitcoin/pull/32783)
💬 bigshiny90 commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021534956)
> @SomberNight hmm.. like this?
>
> Here's the fix needed in `src/rpc/txoutproof.cpp` in the `verifytxoutproof` function after line 220:
>
> ```c++
> const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
>
> // Extract witness nonce from coinbase input
> if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
> return {};
> }
> const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitnes
...
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021534956)
> @SomberNight hmm.. like this?
>
> Here's the fix needed in `src/rpc/txoutproof.cpp` in the `verifytxoutproof` function after line 220:
>
> ```c++
> const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
>
> // Extract witness nonce from coinbase input
> if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
> return {};
> }
> const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitnes
...
💬 bigshiny90 commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2176303594)
@SomberNight hmm.. like this?
Here's the fix needed in `src/rpc/txoutproof.cpp` in the `verifytxoutproof` function after line 220:
```cpp
const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
return {};
}
const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitness.stack[0];
if (witness
...
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2176303594)
@SomberNight hmm.. like this?
Here's the fix needed in `src/rpc/txoutproof.cpp` in the `verifytxoutproof` function after line 220:
```cpp
const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_gentx->vin[0].scriptWitness.stack.empty()) {
return {};
}
const auto& witness_nonce = merkleBlock.m_gentx->vin[0].scriptWitness.stack[0];
if (witness
...
🤔 ryanofsky reviewed a pull request: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345#pullrequestreview-2973346180)
Rebased 3ed3f7f979fb8e4cf3023deb64acf9b0a545057a -> 5024f552924b214fa8a092c1daf9acf92a4c40f0 ([`pr/ipc-stop.11`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.11) -> [`pr/ipc-stop.12`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-stop.11-rebase..pr/ipc-stop.12)) with update to fix tidy errors and try to incorporate https://github.com/bitcoin-core/libmultiprocess/pull/186
Will mark as a draft since this n
...
(https://github.com/bitcoin/bitcoin/pull/32345#pullrequestreview-2973346180)
Rebased 3ed3f7f979fb8e4cf3023deb64acf9b0a545057a -> 5024f552924b214fa8a092c1daf9acf92a4c40f0 ([`pr/ipc-stop.11`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.11) -> [`pr/ipc-stop.12`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-stop.11-rebase..pr/ipc-stop.12)) with update to fix tidy errors and try to incorporate https://github.com/bitcoin-core/libmultiprocess/pull/186
Will mark as a draft since this n
...
💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2176329943)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2171026682
> CI on [Sjors#90 (comment)](https://github.com/Sjors/bitcoin/pull/90#issuecomment-3011857912) complains of an unused result. Which is odd because the `(void)` cast should prevent such a warning. This seems to be a gcc thing: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
Thanks! I tried to work around this by adding a `!` though reading gcc issue maybe that will not be sufficient.
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2176329943)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2171026682
> CI on [Sjors#90 (comment)](https://github.com/Sjors/bitcoin/pull/90#issuecomment-3011857912) complains of an unused result. Which is odd because the `(void)` cast should prevent such a warning. This seems to be a gcc thing: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
Thanks! I tried to work around this by adding a `!` though reading gcc issue maybe that will not be sufficient.
💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2176327196)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2171082503
> `<csignal>`
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2176327196)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2171082503
> `<csignal>`
Thanks, fixed
📝 ryanofsky converted_to_draft a pull request: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345)
This PR fixes various problems when IPC connections are broken or hang which were reported in https://github.com/bitcoin-core/libmultiprocess/issues/123, https://github.com/bitcoin-core/libmultiprocess/issues/176, and https://github.com/bitcoin-core/libmultiprocess/pull/182. The different fixes are described in commit messages.
---
The first two commits of this PR update the libmultiprocess subtree including the following PRs:
- https://github.com/bitcoin-core/libmultiprocess/pull/181
...
(https://github.com/bitcoin/bitcoin/pull/32345)
This PR fixes various problems when IPC connections are broken or hang which were reported in https://github.com/bitcoin-core/libmultiprocess/issues/123, https://github.com/bitcoin-core/libmultiprocess/issues/176, and https://github.com/bitcoin-core/libmultiprocess/pull/182. The different fixes are described in commit messages.
---
The first two commits of this PR update the libmultiprocess subtree including the following PRs:
- https://github.com/bitcoin-core/libmultiprocess/pull/181
...