👍 rkrux approved a pull request: "test: avoid treating hash results as integers (part 1)"
(https://github.com/bitcoin/bitcoin/pull/32050#pullrequestreview-2681875100)
Concept and utACK a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc
> the only exceptions I could think of is PoW-verification of block hashes with the less-than (<) operator
As soon as I read the PR title, I immediately thought of this^ use case but I didn't know there were not more such use cases like this.
> But that would of course need larger, potentially more controversial changes, and its questionable if its really worth the effort.
Agree, good decision to clean up only in the tests fi
...
(https://github.com/bitcoin/bitcoin/pull/32050#pullrequestreview-2681875100)
Concept and utACK a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc
> the only exceptions I could think of is PoW-verification of block hashes with the less-than (<) operator
As soon as I read the PR title, I immediately thought of this^ use case but I didn't know there were not more such use cases like this.
> But that would of course need larger, potentially more controversial changes, and its questionable if its really worth the effort.
Agree, good decision to clean up only in the tests fi
...
💬 rkrux commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993503185)
bytes -> int and then int -> bytes
type conversion is distracting anyway, all the more if not necessary
going away with this change, +1
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993503185)
bytes -> int and then int -> bytes
type conversion is distracting anyway, all the more if not necessary
going away with this change, +1
💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2721308708)
re-ACK 8ceaecc89e20129f9c11727e4c7795633cfcdd2e [08477fd..8ceaec](https://github.com/bitcoin/bitcoin/compare/f4344220d7195324f921dcf001c1a117008477fd..8ceaecc89e20129f9c11727e4c7795633cfcdd2e)
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2721308708)
re-ACK 8ceaecc89e20129f9c11727e4c7795633cfcdd2e [08477fd..8ceaec](https://github.com/bitcoin/bitcoin/compare/f4344220d7195324f921dcf001c1a117008477fd..8ceaecc89e20129f9c11727e4c7795633cfcdd2e)
🤔 ismaelsadeeq reviewed a pull request: "doc: add guidance for RPC to developer notes"
(https://github.com/bitcoin/bitcoin/pull/30142#pullrequestreview-2681978893)
ACK 40ac07ef45f6de0ac6a13e34797387a6ee140de9
(https://github.com/bitcoin/bitcoin/pull/30142#pullrequestreview-2681978893)
ACK 40ac07ef45f6de0ac6a13e34797387a6ee140de9
💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1993642671)
It depends on how much effort you want to put on it.
Ideally, the function signature should clearly indicate if the procedure can fail or not. Exceptions should be reserved for unexpected situations. In this case, we're throwing an specific exception in an internal wallet function solely to notify the user about an invalid parameter provided by the RPC interface. This creates unintended coupling between components (the RPC code needs to knows about the internal wallet function throwing this spe
...
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1993642671)
It depends on how much effort you want to put on it.
Ideally, the function signature should clearly indicate if the procedure can fail or not. Exceptions should be reserved for unexpected situations. In this case, we're throwing an specific exception in an internal wallet function solely to notify the user about an invalid parameter provided by the RPC interface. This creates unintended coupling between components (the RPC code needs to knows about the internal wallet function throwing this spe
...
💬 ismaelsadeeq commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1993586159)
Pass `onfirmed_only` to ensure that the transaction descends from a different coinbase.
Additionally, you can go further by ensuring that you have at least two confirmed UTXOs at the beginning; otherwise, split the first coinbase.
```suggestion
tx_d = self.wallet.send_self_transfer(from_node=node,
fee_rate=Decimal("0.00100"), confirmed_only=True)
```
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1993586159)
Pass `onfirmed_only` to ensure that the transaction descends from a different coinbase.
Additionally, you can go further by ensuring that you have at least two confirmed UTXOs at the beginning; otherwise, split the first coinbase.
```suggestion
tx_d = self.wallet.send_self_transfer(from_node=node,
fee_rate=Decimal("0.00100"), confirmed_only=True)
```
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993657612)
> where we're checking the raw n value
That is to check user input.
I think the abstraction level here is fine, but happy to review if you propose a further abstraction in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993657612)
> where we're checking the raw n value
That is to check user input.
I think the abstraction level here is fine, but happy to review if you propose a further abstraction in a follow-up.
💬 theStack commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993663031)
Agree. My gut-feeling preference would be to remove the caching both for txid and wtxids, as I guess it doesn't make a notable performance difference (likely most of the time in functional tests is spent waiting for RPC results), but have to verify that assumption first.
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993663031)
Agree. My gut-feeling preference would be to remove the caching both for txid and wtxids, as I guess it doesn't make a notable performance difference (likely most of the time in functional tests is spent waiting for RPC results), but have to verify that assumption first.
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993676044)
Not sure but I think this would cause a conversion from size_t to int8_t on each iteration, rather than once for `return i`. Doesn't seem worth it.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993676044)
Not sure but I think this would cause a conversion from size_t to int8_t on each iteration, rather than once for `return i`. Doesn't seem worth it.
🤔 rkrux reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2682207939)
Concept ACK a3a4d199e298a76725d0c0424195ae54c7e95fe0
I'm in favour of adding these tests. I've not reviewed the PR in detail but only glanced as of now. I'd be quick to complete the review once few of the outstanding feedback is addressed. Specially this one: https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1856607483, this https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1858636182 has a few interesting ideas as well.
Re: https://github.com/bitcoin/bitcoin/pull/31340#pul
...
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2682207939)
Concept ACK a3a4d199e298a76725d0c0424195ae54c7e95fe0
I'm in favour of adding these tests. I've not reviewed the PR in detail but only glanced as of now. I'd be quick to complete the review once few of the outstanding feedback is addressed. Specially this one: https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1856607483, this https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1858636182 has a few interesting ideas as well.
Re: https://github.com/bitcoin/bitcoin/pull/31340#pul
...
💬 mabu44 commented on pull request "contrib: Fix `gen-bitcoin-conf.sh`":
(https://github.com/bitcoin/bitcoin/pull/32049#issuecomment-2721511568)
Tested ACK a24419f8bed5e1145ce171dbbdad957750585471
(https://github.com/bitcoin/bitcoin/pull/32049#issuecomment-2721511568)
Tested ACK a24419f8bed5e1145ce171dbbdad957750585471
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993689355)
Yes, IIRC `std::optional` wasn't available when this was added, as the codebase was using C++11 at that time. I think this would be more of a modernization rewrite than a cleanup, but happy to review if you want to propose it.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993689355)
Yes, IIRC `std::optional` wasn't available when this was added, as the codebase was using C++11 at that time. I think this would be more of a modernization rewrite than a cleanup, but happy to review if you want to propose it.
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993710323)
If I'm understanding the comment ("simple structs are fine"), it comes from the idea of structs being for POD, say, in C. In C++, classes and structs are identical except for their default inheritance and member access levels, and for the use cases here, a struct appears to be the more appropriate choice that allows simpler, cleaner code (see also [stackoverflow.com/a/46339933](https://stackoverflow.com/a/46339933). Struct inheritance is also fine (see discussions like [stackoverflow.com/questio
...
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993710323)
If I'm understanding the comment ("simple structs are fine"), it comes from the idea of structs being for POD, say, in C. In C++, classes and structs are identical except for their default inheritance and member access levels, and for the use cases here, a struct appears to be the more appropriate choice that allows simpler, cleaner code (see also [stackoverflow.com/a/46339933](https://stackoverflow.com/a/46339933). Struct inheritance is also fine (see discussions like [stackoverflow.com/questio
...
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2721564695)
> I like these cleanups, ans we should go a step further and modernize the code more when touching them. I'm also not a fan of the 3rd commit for the inheritance cases and would prefer seeing the last commit in a separate PR, doesn't seem like a "refactor:"
The last commit was added at the request of multiple reviewers, but I've removed "refactor" from the pull title. Addressed the other feedback individually and happy to look at any modernization follow-ups you'd like to propose.
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2721564695)
> I like these cleanups, ans we should go a step further and modernize the code more when touching them. I'm also not a fan of the 3rd commit for the inheritance cases and would prefer seeing the last commit in a separate PR, doesn't seem like a "refactor:"
The last commit was added at the request of multiple reviewers, but I've removed "refactor" from the pull title. Addressed the other feedback individually and happy to look at any modernization follow-ups you'd like to propose.
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721565218)
Concept ACK (again)
It would be good to mention #25717 in the PR description.
Digging through older issues, I found a few references to the function of checkpoints.
https://github.com/bitcoin/bitcoin/issues/4512#issuecomment-48818097:
> With headers-first, checkpoint-based sigcheck skipping can be replaced by a "do not verify signatures when buried beneath enough PoW". We'll need a weaker form of checkpoints still to prevent the "fill your memory with a ton of low-difficulty headers
...
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721565218)
Concept ACK (again)
It would be good to mention #25717 in the PR description.
Digging through older issues, I found a few references to the function of checkpoints.
https://github.com/bitcoin/bitcoin/issues/4512#issuecomment-48818097:
> With headers-first, checkpoint-based sigcheck skipping can be replaced by a "do not verify signatures when buried beneath enough PoW". We'll need a weaker form of checkpoints still to prevent the "fill your memory with a ton of low-difficulty headers
...
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993724292)
It was added at the request of multiple reviewers and doesn't seem worth splitting out to another PR for that one line and requiring an additional merge script run. Instead, I've dropped the word "refactor" from the PR title.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993724292)
It was added at the request of multiple reviewers and doesn't seem worth splitting out to another PR for that one line and requiring an additional merge script run. Instead, I've dropped the word "refactor" from the PR title.
💬 jonatack commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1993747780)
```suggestion
- It's preferable to avoid changing an RPC in a backward-incompatible manner, but in that case, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data type changes (e.g. from `{"warnings":""}` to `{"warnings":[]}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"warning":""}` to `{"warnings":""}`). Adding a key to an obj
...
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1993747780)
```suggestion
- It's preferable to avoid changing an RPC in a backward-incompatible manner, but in that case, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data type changes (e.g. from `{"warnings":""}` to `{"warnings":[]}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"warning":""}` to `{"warnings":""}`). Adding a key to an obj
...
👋 jonatack's pull request is ready for review: "p2p: protect addnode peers during IBD"
(https://github.com/bitcoin/bitcoin/pull/32051)
(https://github.com/bitcoin/bitcoin/pull/32051)
💬 mzumsande commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2721637434)
> so not sure if a fix would conflict with that?
Haven't looked into this one yet, but it shouldn't really matter (#31829) if it conflicts or not - a fix should have priority, if possible be included in 29.0, so that we don't have a release with frequent intermittent timeouts.
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2721637434)
> so not sure if a fix would conflict with that?
Haven't looked into this one yet, but it shouldn't really matter (#31829) if it conflicts or not - a fix should have priority, if possible be included in 29.0, so that we don't have a release with frequent intermittent timeouts.
💬 marcofleon commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721641157)
Rebased after 44041ae0eca9d2034b7c2bdef24724809cc35e90 and added https://github.com/bitcoin/bitcoin/pull/25717 in the description.
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721641157)
Rebased after 44041ae0eca9d2034b7c2bdef24724809cc35e90 and added https://github.com/bitcoin/bitcoin/pull/25717 in the description.
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721646233)
Code review ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
If #31974 lands then this PR will be more difficult to revert. So it might be marginally better to drop the checkpoints in a separate commit. Here's a [commit sequence](https://github.com/Sjors/bitcoin/commits/2025/03/checkpoint-removal/) that would work.
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721646233)
Code review ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
If #31974 lands then this PR will be more difficult to revert. So it might be marginally better to drop the checkpoints in a separate commit. Here's a [commit sequence](https://github.com/Sjors/bitcoin/commits/2025/03/checkpoint-removal/) that would work.