💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2822126808)
Rebased 720f253abbbeb56872b6c16deee26f3fab842254 -> 4a4eeb94339bb9200012df6a57769dc28e35f553 ([kernelApi_36](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_36) -> [kernelApi_37](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_37), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_36..kernelApi_37))
* Fixed conflict with #32308
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2822126808)
Rebased 720f253abbbeb56872b6c16deee26f3fab842254 -> 4a4eeb94339bb9200012df6a57769dc28e35f553 ([kernelApi_36](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_36) -> [kernelApi_37](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_37), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_36..kernelApi_37))
* Fixed conflict with #32308
🤔 w0xlt reviewed a pull request: "kernel: Separate UTXO set access from validation functions"
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2785006540)
Concept ACK, as this PR increases the decoupling of components.
nit: The name `SpendBlock` may not be clear (unless this concept is already known and I'm missing the point).
Maybe `BIP30Validation`, something like that, would be clearer.
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2785006540)
Concept ACK, as this PR increases the decoupling of components.
nit: The name `SpendBlock` may not be clear (unless this concept is already known and I'm missing the point).
Maybe `BIP30Validation`, something like that, would be clearer.
💬 maflcko commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054677515)
nit in 7f368ea62cd89e6ff4238ec49efa2eaddd0e5046: Needs rebase, as it is already included in master. I think the name already implies that this is the "highest sequence number with the given property (non-final)", so the comment seems redundant. The comment seems better to mention how it relates to BIPs. For reference, the existing comment is: `Sequence number that is csv-opt-out (BIP 68)`. See: https://github.com/bitcoin/bitcoin/commit/06439a14c884d7f81f331646ad361e88b3037a51#diff-1698fabd4ddd29
...
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054677515)
nit in 7f368ea62cd89e6ff4238ec49efa2eaddd0e5046: Needs rebase, as it is already included in master. I think the name already implies that this is the "highest sequence number with the given property (non-final)", so the comment seems redundant. The comment seems better to mention how it relates to BIPs. For reference, the existing comment is: `Sequence number that is csv-opt-out (BIP 68)`. See: https://github.com/bitcoin/bitcoin/commit/06439a14c884d7f81f331646ad361e88b3037a51#diff-1698fabd4ddd29
...
💬 w0xlt commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054679774)
```suggestion
bool CConnman::OutboundConnectedToAddress(const std::string& str_addr)
```
The caller already knows that the parameter is a string type, but clarifying what the string represents (a network address) seems better to me.
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2054679774)
```suggestion
bool CConnman::OutboundConnectedToAddress(const std::string& str_addr)
```
The caller already knows that the parameter is a string type, but clarifying what the string represents (a network address) seems better to me.
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2054684233)
> @mzumsande, any thoughts on how to treat a timeout after we sent `INV` (no `GETDATA` request)? Treat as send failure and schedule a new connection or treat as successful send and do not schedule a new connection?
The arguments from @andrewtoth make sense to me. I think all situations where we'd like to treat it as a send failure/schedule another connection involve a peer that is either broken in an unusual way or malicious. Malicious peers can act as a black hole, so they can be ignored her
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2054684233)
> @mzumsande, any thoughts on how to treat a timeout after we sent `INV` (no `GETDATA` request)? Treat as send failure and schedule a new connection or treat as successful send and do not schedule a new connection?
The arguments from @andrewtoth make sense to me. I think all situations where we'd like to treat it as a send failure/schedule another connection involve a peer that is either broken in an unusual way or malicious. Malicious peers can act as a black hole, so they can be ignored her
...
💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2822219540)
> ACK [dda90a9](https://github.com/bitcoin/bitcoin/commit/dda90a96bd01e34f4a564fbd15f1948f1c22ff58)
>
> Nice find!
>
> > external non-ranged descriptor MUST be able to have label (current impl disallows it)
>
> Since we already have a test for this and this PR doesn't change it, could you clarify what you mean?
the test in question:
```
self.log.info("Should import a p2pkh descriptor")
import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"),
...
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2822219540)
> ACK [dda90a9](https://github.com/bitcoin/bitcoin/commit/dda90a96bd01e34f4a564fbd15f1948f1c22ff58)
>
> Nice find!
>
> > external non-ranged descriptor MUST be able to have label (current impl disallows it)
>
> Since we already have a test for this and this PR doesn't change it, could you clarify what you mean?
the test in question:
```
self.log.info("Should import a p2pkh descriptor")
import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"),
...
💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2822221513)
re-based on current master
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2822221513)
re-based on current master
💬 l0rinc commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2822227361)
ACK faca46b0421b568e7e5fefe593420e773d0ec9af
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2822227361)
ACK faca46b0421b568e7e5fefe593420e773d0ec9af
🤔 rkrux reviewed a pull request: "test: fix pushdata scripts"
(https://github.com/bitcoin/bitcoin/pull/32270#pullrequestreview-2785087703)
Concept ACK
My bad I didn't look at the scripts thoroughly in the previous PR and focussed more on the design of the examples.
I debugged the scripts in the test and can indeed see the unintended script as described in this comment: https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2034051063
```
('encodeable_pushdata1', CScript([OP_DROP, x('96616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616
...
(https://github.com/bitcoin/bitcoin/pull/32270#pullrequestreview-2785087703)
Concept ACK
My bad I didn't look at the scripts thoroughly in the previous PR and focussed more on the design of the examples.
I debugged the scripts in the test and can indeed see the unintended script as described in this comment: https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2034051063
```
('encodeable_pushdata1', CScript([OP_DROP, x('96616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616
...
💬 rkrux commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054706420)
Shouldn't this one be called `nonstd_2byte_pushdata1` because it uses the `PUSHDATA1` opcode?
That's what is mentioned in the comments of the previous PR and in the spender comment down below.
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054706420)
Shouldn't this one be called `nonstd_2byte_pushdata1` because it uses the `PUSHDATA1` opcode?
That's what is mentioned in the comments of the previous PR and in the spender comment down below.
💬 rkrux commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054707416)
I'm guessing it's intentional to have two different ways of creating `CScript` here?
For better illustration.
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054707416)
I'm guessing it's intentional to have two different ways of creating `CScript` here?
For better illustration.
💬 maflcko commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2822268104)
For reference, in the two CI `DEBUG=1` configurations, the bench part seems to take 3x the time. Though, this should be fine, given the motivation in the pull description, and also the fact that there are similarly slow other tests.
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2822268104)
For reference, in the two CI `DEBUG=1` configurations, the bench part seems to take 3x the time. Though, this should be fine, given the motivation in the pull description, and also the fact that there are similarly slow other tests.
💬 TheCharlatan commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054728640)
The comment may be confusing in the context of just using nlocktime, so maybe it should be amended to describe its non-BIP68 meaning too?
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054728640)
The comment may be confusing in the context of just using nlocktime, so maybe it should be amended to describe its non-BIP68 meaning too?
💬 maflcko commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054735178)
e78e92f6363c4ea1cbedd6631fe56c0cd8e164d5: I don't think this is right. Why the `-1`?
For reference, I debugged this via:
```diff
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 13f6088192..927fcb483c 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -92,6 +92,7 @@ void initialize_chain()
Assert(processed);
const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockInd
...
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054735178)
e78e92f6363c4ea1cbedd6631fe56c0cd8e164d5: I don't think this is right. Why the `-1`?
For reference, I debugged this via:
```diff
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 13f6088192..927fcb483c 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -92,6 +92,7 @@ void initialize_chain()
Assert(processed);
const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockInd
...
💬 maflcko commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054746098)
Yeah, the comments here are one-line summaries of the "real" comments in `./src/primitives/transaction.h` only. A third alternative may be to just remove them and refer to the docs in `./src/primitives/transaction.h` to avoid having to copy paste the full docs into both places every time they are touched.
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054746098)
Yeah, the comments here are one-line summaries of the "real" comments in `./src/primitives/transaction.h` only. A third alternative may be to just remove them and refer to the docs in `./src/primitives/transaction.h` to avoid having to copy paste the full docs into both places every time they are touched.
🤔 BrandonOdiwuor reviewed a pull request: "test: Run all benchmarks in the sanity check"
(https://github.com/bitcoin/bitcoin/pull/32310#pullrequestreview-2785224059)
Code Review ACK faca46b0421b568e7e5fefe593420e773d0ec9af
(https://github.com/bitcoin/bitcoin/pull/32310#pullrequestreview-2785224059)
Code Review ACK faca46b0421b568e7e5fefe593420e773d0ec9af
💬 instagibbs commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054795871)
The python framework won't let you construct a non-standard serialization natively, AFAIK. So we're doing it manually.
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054795871)
The python framework won't let you construct a non-standard serialization natively, AFAIK. So we're doing it manually.
💬 instagibbs commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054797532)
I was focusing on two different ways of pushing 2 bytes, not naming the specific opcode. If it would help legibility I can re-introduce the exact pushdata opcode name in use.
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054797532)
I was focusing on two different ways of pushing 2 bytes, not naming the specific opcode. If it would help legibility I can re-introduce the exact pushdata opcode name in use.
💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2054831851)
Is this assertion correct - why can't the best blockhash be B8?
Even though B7 is received first, both are put into `std::multimap<CBlockIndex*, CBlockIndex*> m_blocks_unlinked;` (with no comparator, so pointer address is used which is decided by the OS?!). and later, when their parent arrives, accessed via `equal_range` in `ReceivedBlockTransactions` where `nSequenceId` is then set in that order. Couldn't this be done in the reverse order B8 -> B7 if the OS gives out the pointer addresses in a
...
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2054831851)
Is this assertion correct - why can't the best blockhash be B8?
Even though B7 is received first, both are put into `std::multimap<CBlockIndex*, CBlockIndex*> m_blocks_unlinked;` (with no comparator, so pointer address is used which is decided by the OS?!). and later, when their parent arrives, accessed via `equal_range` in `ReceivedBlockTransactions` where `nSequenceId` is then set in that order. Couldn't this be done in the reverse order B8 -> B7 if the OS gives out the pointer addresses in a
...
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2054848643)
I agree this is pretty intricate. I'll try to explain in the comments, to see if you have any suggestions for making it clearer.
The `SetType modified;` variable contains a conservative *overestimate* of which clusters in the real graph may differ between main and staging. Whenever a transaction is added, or removed, or undergoes a dependency adding in the simulated staging graph, it is definitely considered modified. But for example, say transactions A->B exists in main & staging, so they're
...
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2054848643)
I agree this is pretty intricate. I'll try to explain in the comments, to see if you have any suggestions for making it clearer.
The `SetType modified;` variable contains a conservative *overestimate* of which clusters in the real graph may differ between main and staging. Whenever a transaction is added, or removed, or undergoes a dependency adding in the simulated staging graph, it is definitely considered modified. But for example, say transactions A->B exists in main & staging, so they're
...