💬 pstratem commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#issuecomment-3123049111)
Ok I thought about it and it just wasn't obviously correct enough.
So I've rewritten into three commits to be simpler.
(https://github.com/bitcoin/bitcoin/pull/32885#issuecomment-3123049111)
Ok I thought about it and it just wasn't obviously correct enough.
So I've rewritten into three commits to be simpler.
💬 pstratem commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2233243382)
It's technically possible for disconnecting a block to get us out of IBD, though I really don't think that particular edge case is super important.
I was just trying to be thorough.
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2233243382)
It's technically possible for disconnecting a block to get us out of IBD, though I really don't think that particular edge case is super important.
I was just trying to be thorough.
💬 murchandamus commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3123344903)
> Well we all know the algorithm is non-deterministic, however ideally it would be possible to make deterministic test cases :).
Such a test would be brittle and tied to the exact implementation details of the algorithm rather than the characteristics of the algorithm. That doesn’t seem like an obvious improvement.
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3123344903)
> Well we all know the algorithm is non-deterministic, however ideally it would be possible to make deterministic test cases :).
Such a test would be brittle and tied to the exact implementation details of the algorithm rather than the characteristics of the algorithm. That doesn’t seem like an obvious improvement.
💬 murchandamus commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233250953)
That’s a disimprovement. Please leave both, because the test failing on `res` being falsy indicates other issues than it failing on the weight check.
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233250953)
That’s a disimprovement. Please leave both, because the test failing on `res` being falsy indicates other issues than it failing on the weight check.
💬 yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3123375190)
> Such a test would be brittle and tied to the exact implementation details of the algorithm rather than the characteristics of the algorithm. That doesn’t seem like an obvious improvement.
This test case has a number of invaluable utxos and some valuable ones. Would it not be better to show that after the min-heap is added, that the implementation has a bias towards selecting valuable utxos? I think these types of implementation details would be valuable to test.
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3123375190)
> Such a test would be brittle and tied to the exact implementation details of the algorithm rather than the characteristics of the algorithm. That doesn’t seem like an obvious improvement.
This test case has a number of invaluable utxos and some valuable ones. Would it not be better to show that after the min-heap is added, that the implementation has a bias towards selecting valuable utxos? I think these types of implementation details would be valuable to test.
💬 achow101 commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3123379929)
> New `-dnsresolver` option: Specify a DNSSEC-validating DNS resolver command to handle BIP-353 lookups
I don't think we should be outsourcing that to another program. Validating DNSSEC signatures is an integral part of BIP 353 and the only way we can guarantee that the proof is valid is for us to receive the prrof and validate it internally, which defeats the purpose of calling another binary.
Unlike with `-signer`, I don't think there's a good reason to use a separate binary for the DNS
...
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3123379929)
> New `-dnsresolver` option: Specify a DNSSEC-validating DNS resolver command to handle BIP-353 lookups
I don't think we should be outsourcing that to another program. Validating DNSSEC signatures is an integral part of BIP 353 and the only way we can guarantee that the proof is valid is for us to receive the prrof and validate it internally, which defeats the purpose of calling another binary.
Unlike with `-signer`, I don't think there's a good reason to use a separate binary for the DNS
...
💬 yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233261494)
Alright I added it back. However in the past we have preferred not to have this extra check: https://github.com/bitcoin/bitcoin/pull/31656
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233261494)
Alright I added it back. However in the past we have preferred not to have this extra check: https://github.com/bitcoin/bitcoin/pull/31656
💬 yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233262726)
> nit: Maybe this BOOST_CHECK is not needed anymore since you're checking the result itself.
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233262726)
> nit: Maybe this BOOST_CHECK is not needed anymore since you're checking the result itself.
💬 brunoerg commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3123397126)
> Looking at corecheck, there are currently 3 other functional tests that run slower than `feature_block.py` (`p2p_segwit.py`, `p2p_opportunistic_1p1c.py` & `mining_getblocktemplate_longpoll.py`). `p2p_opportunistic_1p1c.py` should be better after #33048.
Sure, I'm taking a look at these other ones as well. I'm addressing `feature_block` for now because it's used a lot on the mutation analysis.
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3123397126)
> Looking at corecheck, there are currently 3 other functional tests that run slower than `feature_block.py` (`p2p_segwit.py`, `p2p_opportunistic_1p1c.py` & `mining_getblocktemplate_longpoll.py`). `p2p_opportunistic_1p1c.py` should be better after #33048.
Sure, I'm taking a look at these other ones as well. I'm addressing `feature_block` for now because it's used a lot on the mutation analysis.
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2233299278)
Thanks, that's another good alternative. Let me know if you insist, but I have already taken your previous approach.
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2233299278)
Thanks, that's another good alternative. Let me know if you insist, but I have already taken your previous approach.
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3123472162)
One advantage of doing a separate process is that it sidesteps either dealing with validation integration [1], which will invariably stall progress ~forever. Using a separate binary for now would let the work streams progress independently and figuring out how to in-house validation can come separately.
[1] I think there's several ways, so even analyzing them will probably stall things ~forever - Rust integration, trying to write secp256r1/RSA plus DNSSEC validation in C++, or taking on some de
...
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3123472162)
One advantage of doing a separate process is that it sidesteps either dealing with validation integration [1], which will invariably stall progress ~forever. Using a separate binary for now would let the work streams progress independently and figuring out how to in-house validation can come separately.
[1] I think there's several ways, so even analyzing them will probably stall things ~forever - Rust integration, trying to write secp256r1/RSA plus DNSSEC validation in C++, or taking on some de
...
💬 achow101 commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3123744788)
> One advantage of doing a separate process is that it sidesteps either dealing with validation integration
It introduces a potential security issue though since we can't validate the output of the separate process. We're just trusting whatever it spits out, and if it were malicious, it could be making fake addresses which results in funds being sent to an attacker., and the user basically wouldn't know.
There's also the question of how to distribute that binary which I think would result
...
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3123744788)
> One advantage of doing a separate process is that it sidesteps either dealing with validation integration
It introduces a potential security issue though since we can't validate the output of the separate process. We're just trusting whatever it spits out, and if it were malicious, it could be making fake addresses which results in funds being sent to an attacker., and the user basically wouldn't know.
There's also the question of how to distribute that binary which I think would result
...
💬 luke-jr commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2233596644)
`{self.config['environment']['CLIENT_NAME']}`
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2233596644)
`{self.config['environment']['CLIENT_NAME']}`
💬 luke-jr commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2233596661)
`{self.config['environment']['CLIENT_NAME']}`
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2233596661)
`{self.config['environment']['CLIENT_NAME']}`
💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2233730246)
@macgyver13 Did you use the same private key for scan and spend?
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2233730246)
@macgyver13 Did you use the same private key for scan and spend?
💬 l0rinc commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233666164)
nit: the code isn't in exact alignment with the comment - and since we *are* calling it with `0`, maybe something like:
```suggestion
Assert(d >= 0ms); // Steady time cannot move backward.
```
(nit2: to obviate that we don't just mean that jumps cannot be 0.5 seconds, we might want to use `0ms` instead)
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233666164)
nit: the code isn't in exact alignment with the comment - and since we *are* calling it with `0`, maybe something like:
```suggestion
Assert(d >= 0ms); // Steady time cannot move backward.
```
(nit2: to obviate that we don't just mean that jumps cannot be 0.5 seconds, we might want to use `0ms` instead)
💬 l0rinc commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233663906)
typo in commit message:
> no state *is* leaked between test cases
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233663906)
typo in commit message:
> no state *is* leaked between test cases
💬 l0rinc commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233658117)
👍 for using the built-in `min` instead.
Could we add it to the commit message that this isn't just a simple refactor (and not just about the return value), but as @hodlinator stated in a different PR where he did the same:
> That way we can always proceed instead of just exiting the fuzz target, wasting cycles. Also reduces code complexity.
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233658117)
👍 for using the built-in `min` instead.
Could we add it to the commit message that this isn't just a simple refactor (and not just about the return value), but as @hodlinator stated in a different PR where he did the same:
> That way we can always proceed instead of just exiting the fuzz target, wasting cycles. Also reduces code complexity.
🤔 l0rinc reviewed a pull request: "test: Add and use ElapseTime helper"
(https://github.com/bitcoin/bitcoin/pull/32430#pullrequestreview-3058931165)
Thanks for fixing these, left quite a few comments, hope you'll find them useful
(https://github.com/bitcoin/bitcoin/pull/32430#pullrequestreview-3058931165)
Thanks for fixing these, left quite a few comments, hope you'll find them useful
💬 l0rinc commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233667188)
nit: I understand that it's meant to be symmetric with `ElapseSteady`, but I don't find the comment helpful as it is.
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233667188)
nit: I understand that it's meant to be symmetric with `ElapseSteady`, but I don't find the comment helpful as it is.