👋 achow101's pull request is ready for review: "wallet: Track no-longer-spendable TXOs separately"
(https://github.com/bitcoin/bitcoin/pull/27865)
(https://github.com/bitcoin/bitcoin/pull/27865)
🤔 w0xlt reviewed a pull request: "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC"
(https://github.com/bitcoin/bitcoin/pull/32593#pullrequestreview-2976415024)
ACK https://github.com/bitcoin/bitcoin/pull/32593/commits/6135e0553e6e58fcf506700991fa178f2c50a266
Moving to (Un)LockCoin, the responsibility logic of creating the `WalletBatch` instance and persisting the data improves code readability and separation of concerns.
(https://github.com/bitcoin/bitcoin/pull/32593#pullrequestreview-2976415024)
ACK https://github.com/bitcoin/bitcoin/pull/32593/commits/6135e0553e6e58fcf506700991fa178f2c50a266
Moving to (Un)LockCoin, the responsibility logic of creating the `WalletBatch` instance and persisting the data improves code readability and separation of concerns.
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3024981580)
@sipa one order of magnitude more for me on my wimpy laptop but well within acceptable bounds I suspect for a reorg. IIUC this is one of the more pessimistic mempool topologies for this since you're going to have to go through the ~entire mempool before finishing
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3024981580)
@sipa one order of magnitude more for me on my wimpy laptop but well within acceptable bounds I suspect for a reorg. IIUC this is one of the more pessimistic mempool topologies for this since you're going to have to go through the ~entire mempool before finishing
🤔 Sjors reviewed a pull request: "RPC/txoutproof: Support including (and verifying) proofs of wtxid"
(https://github.com/bitcoin/bitcoin/pull/32844#pullrequestreview-2976408858)
Took an initial look at the code, but got a bit confused. Will check again later.
(https://github.com/bitcoin/bitcoin/pull/32844#pullrequestreview-2976408858)
Took an initial look at the code, but got a bit confused. Will check again later.
💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178198327)
605b372b1f62c4cd59d8056f0a6a1baf17c24014: to avoid confusion between an _empty_ block and a pre-segwit block, could comment:
```
// Block without coinbase
```
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178198327)
605b372b1f62c4cd59d8056f0a6a1baf17c24014: to avoid confusion between an _empty_ block and a pre-segwit block, could comment:
```
// Block without coinbase
```
💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178194140)
605b372b1f62c4cd59d8056f0a6a1baf17c24014: `*Assert(block)`
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178194140)
605b372b1f62c4cd59d8056f0a6a1baf17c24014: `*Assert(block)`
💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178192051)
605b372b1f62c4cd59d8056f0a6a1baf17c24014 nit: maybe rename `tx` to `coinbase`
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178192051)
605b372b1f62c4cd59d8056f0a6a1baf17c24014 nit: maybe rename `tx` to `coinbase`
💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178215585)
3abfd6d3d99bbcceb09162e83d8f9f9769bd4036:
```cpp
if (prove_witness) {
if (i == 0) {
// generation tx has null wtxid
wtxids.emplace_back();
} else {
wtxids.push_back(block.vtx[i]->GetWitnessHash());
}
```
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178215585)
3abfd6d3d99bbcceb09162e83d8f9f9769bd4036:
```cpp
if (prove_witness) {
if (i == 0) {
// generation tx has null wtxid
wtxids.emplace_back();
} else {
wtxids.push_back(block.vtx[i]->GetWitnessHash());
}
```
💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178222894)
3abfd6d3d99bbcceb09162e83d8f9f9769bd4036: what does this do?
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178222894)
3abfd6d3d99bbcceb09162e83d8f9f9769bd4036: what does this do?
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178238384)
Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/24539/commits/d5980f39f934a8e251ddaf6801d23bd5a4bcb3ef
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178238384)
Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/24539/commits/d5980f39f934a8e251ddaf6801d23bd5a4bcb3ef
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178238458)
Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/24539/commits/d5980f39f934a8e251ddaf6801d23bd5a4bcb3ef
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178238458)
Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/24539/commits/d5980f39f934a8e251ddaf6801d23bd5a4bcb3ef
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178244200)
done in https://github.com/bitcoin/bitcoin/pull/24539/commits/d5980f39f934a8e251ddaf6801d23bd5a4bcb3ef. Moving this does not work (maybe a conflict with the mempool lock ?) I'll investigate.
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178244200)
done in https://github.com/bitcoin/bitcoin/pull/24539/commits/d5980f39f934a8e251ddaf6801d23bd5a4bcb3ef. Moving this does not work (maybe a conflict with the mempool lock ?) I'll investigate.
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178248747)
Correct but I wanted to have 2 explicit tests: one when a spending tx is replaced, the other when it is cancelled.
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178248747)
Correct but I wanted to have 2 explicit tests: one when a spending tx is replaced, the other when it is cancelled.
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178249989)
Sorry I don't understand this ?
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178249989)
Sorry I don't understand this ?
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3025042997)
> In the pull request description you are saying that the keys are 64 bytes, but I guess you mean 64 bits?
Yes 64 bits.
>
> Did you split this work into two commits to gather feedback on tradeoffs between the two approaches? The space savings and the ability to immediately return a transaction is nice, but it is also not compatible with pruning (and still requires notices about that in init.cpp). Some lightning implementations do support pruned nodes, I guess they won't be able to leverage t
...
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3025042997)
> In the pull request description you are saying that the keys are 64 bytes, but I guess you mean 64 bits?
Yes 64 bits.
>
> Did you split this work into two commits to gather feedback on tradeoffs between the two approaches? The space savings and the ability to immediately return a transaction is nice, but it is also not compatible with pruning (and still requires notices about that in init.cpp). Some lightning implementations do support pruned nodes, I guess they won't be able to leverage t
...
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178267285)
It may help user figure out they've made a mistake in their setup/configuration ?
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178267285)
It may help user figure out they've made a mistake in their setup/configuration ?
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178272887)
It did help when the index returned a tx id, it does not anymore.
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178272887)
It did help when the index returned a tx id, it does not anymore.
⚠️ henark opened an issue: "refactor: Standardize logging levels and increase granularity in P2P and mempool modules"
(https://github.com/bitcoin/bitcoin/issues/32851)
Problem: Analysis of developer notes shows five log levels (LogDebug, LogInfo, LogError, LogWarning, LogTrace), but their application can be inconsistent. Critical modules like P2P message handling and mempool acceptance logic lack sufficient LogTrace or fine-grained LogDebug messages, making it difficult to debug complex emergent behaviors like those seen in recent DoS vulnerabilities (CVE-2024-52914, CVE-2024-35202) without attaching a full debugger.
Proposal: Systematically review and refact
...
(https://github.com/bitcoin/bitcoin/issues/32851)
Problem: Analysis of developer notes shows five log levels (LogDebug, LogInfo, LogError, LogWarning, LogTrace), but their application can be inconsistent. Critical modules like P2P message handling and mempool acceptance logic lack sufficient LogTrace or fine-grained LogDebug messages, making it difficult to debug complex emergent behaviors like those seen in recent DoS vulnerabilities (CVE-2024-52914, CVE-2024-35202) without attaching a full debugger.
Proposal: Systematically review and refact
...
⚠️ henark opened an issue: "test: Create and maintain a test coverage risk matrix for consensus-critical modules"
(https://github.com/bitcoin/bitcoin/issues/32852)
Problem: While coverage reports exist, there is no formal process for prioritizing test development based on risk. Academic analysis and CVE history show that bugs in high-complexity, consensus-critical code are the most dangerous. A latent bug in a low-coverage area of validation.cpp is far more severe than one in the GUI.
Proposal: Introduce a documented process and associated tooling (e.g., a CI script) that combines code complexity metrics (e.g., cyclomatic complexity) with test coverage da
...
(https://github.com/bitcoin/bitcoin/issues/32852)
Problem: While coverage reports exist, there is no formal process for prioritizing test development based on risk. Academic analysis and CVE history show that bugs in high-complexity, consensus-critical code are the most dangerous. A latent bug in a low-coverage area of validation.cpp is far more severe than one in the GUI.
Proposal: Introduce a documented process and associated tooling (e.g., a CI script) that combines code complexity metrics (e.g., cyclomatic complexity) with test coverage da
...
⚠️ henark opened an issue: "p2p: Implement proactive check for stale connections to mitigate eclipse attack vectors"
(https://github.com/bitcoin/bitcoin/issues/32853)
Problem: Eclipse attacks rely on monopolizing a node's connection slots. While Bitcoin Core rotates connections, a sophisticated attacker can attempt to keep connections open but unresponsive, or feed them low-utility data to prevent them from being dropped. The current 90-minute inactivity timeout may be too long.
Proposal: Implement a more proactive health check for peer connections. Beyond the simple ping message, this could involve a challenge-response mechanism that lightly queries the pee
...
(https://github.com/bitcoin/bitcoin/issues/32853)
Problem: Eclipse attacks rely on monopolizing a node's connection slots. While Bitcoin Core rotates connections, a sophisticated attacker can attempt to keep connections open but unresponsive, or feed them low-utility data to prevent them from being dropped. The current 90-minute inactivity timeout may be too long.
Proposal: Implement a more proactive health check for peer connections. Beyond the simple ping message, this could involve a challenge-response mechanism that lightly queries the pee
...