💬 brunoerg commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2273601377)
I didn't review it in depth but I used this PR to test my "mutation testing tool", which creates mutations based on diff, and the tests are REALLY great! The only two mutants that were not killed seems to be "equivalent mutants", so we can ignore them. See:

------------------------------
#### Bitcoin Core (#30535 - feefrac: add support for evaluating at given size)

* `make && ./src/test/test_bitcoin --run_test=feefrac_tests`

* Mutation score: **94.4%** (**Excelent**)

--------------
...

💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-2273618214)
> Coming back to this and added it to my list of things to review. Is there an upstream PR this one depends on that I should be reviewing first, or is there another reason the PR is in draft?

Thanks for being interested in this! The main reason it is in draft i just that it is too big and and I need to work on a way to split it up. For example the first commits and documentation and removing racy -reindex-chainstate behavior should be a separate PR. And I think I want to make a separate PR sq
...

💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2273621519)
Thanks for the review everyone. Maintainers: there is still a determinism issue here as per Niklas, which i have yet to re-reproduce.

-------- Original Message --------
On 8/7/24 4:16 PM, Bruno Garcia wrote:

> @brunoerg commented on this pull request.
>
> ---------------------------------------------------------------
>
> In [src/test/fuzz/block_index.cpp](https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707092262):
>
>> +
> +#include <chain.h>
> +#include <chainparams.h>
> +#includ
...

💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2273637902)
@brunoerg Cool!

💬 kevkevinpal commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2273645530)
> Are you still working on this?

Yup just haven't had much activity on it but rebased to [1d72266](https://github.com/bitcoin/bitcoin/pull/29500/commits/1d722660a65522539872c09ae8a3ba8c9ca55b77)

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2273648419)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2273166427

Thanks! Added following to the description:

> Note: The changes in this PR allow a new `bitcoin-node` process to be started with an `-ipcfd=<n>` argument specifying a file descriptor from a socketpair, allowing another process to control it over the socketpair. This PR by itself is fairly inflexible but combined with #30509 it allows existing `bitcoin-node` processes to be controlled over a unix socket. This PR is al
...

💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707159683)
> I guess because I feel it's hard to make concrete promises about something that far out.

Ok, I don't see it as a promise but as a declared intention. Users don't gain anything from us removing a feature, they could all just stop using it without it being removed so we don't need to promise them the removal. We rather do that for ourselves to relieve ourselves of the maintenance burden. We intended to do that for a specific version unless someone comes forward and makes a case that convinces
...

🤔 stickies-v reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2224783283)
Thanks for the review @paplorinc, force pushed to address all outstanding comments. Style-only changes:
- [introduced](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706884207) `final_size` var to avoid typecasting between (un)signed types and some related cleanup
- [removed](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706891172) hardcoded `64` value
- [cleaned up](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706897088) the `-noassumevalid` logic a bit
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706949955)
> is to make sure we don't have memory problems, don't throw unexpected exceptions, etc?

That's my understanding too.

> would it make sense to compare their outputs, to make sure we at least have internal consistency?

Sounds reasonable, but I think I'll leave that for another PR.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706936276)
I don't have a strong view on it, but this commit just reuses the existing `IsHexNumber` tests so I'd rather not change that here.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706949124)
Makes sense, taken, thanks.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706931724)
> though this contains an extra evennes requirement which we may not want here.

Exactly

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706935424)
Yes, since we're dealing with numbers specifically.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706946286)
Yeah I like your approach better, taken, thanks.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707005179)
Yeah that'll work, ~taken, thanks.

👍 paplorinc approved a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2225466836)
ACK d045fc7ac1729cf29140c43518f20375f2aaa1cc

Note that I don't know the second order effects of constraining previous public input values, somebody else should assess that part

💬 gmaxwell commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2273685985)
Running this exposes that nodes are currently making duplicate transaction requests not too infrequently. ::facepalm:: What happens is that when Peer1 announces {parent,child} and Peer2 announces {parent,child} and we request parent from 1 and child from 2, but 2 responds faster, then orphan handling will request parent by txid from 2. Then it gets the parent by txid from 2. Then when peer 1 replies to the request for parent (sending a duplicate transaction), the current behavior of this PR f
...

👍 tdb3 approved a pull request: "ci: Silent Homebrew's reinstall warnings"
(https://github.com/bitcoin/bitcoin/pull/30591#pullrequestreview-2225476443)
cr ut ACK 032ebe5be4dfc3eb3d3693f1284fac6458bacbf3

💬 maflcko commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2273721994)
I think there was a slight miscommunication. My reply was to your point of a "large fork", whose check (currently) happens after the chain params are asked for the hash: https://github.com/bitcoin/bitcoin/blob/676abd1af754964858a60fddffb9a12c0a6c2749/src/validation.cpp#L5663 .

You also raised a different point about a user downloading a too recent snapshot. This is a different scenario, however the same check in validation will hit it currently. I agree that the error message can be improved
...

💬 maflcko commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2273742263)
> Thanks for the review everyone. Maintainers: there is still a determinism issue here as per Niklas, which i have yet to re-reproduce.

Are you sure? I think may be confused with https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2258390205 ?

Even if not, I wonder if stability is a hard blocker for a fuzz target. Obviously, it should be fixed, but this can be done in a follow-up, if the fuzz target otherwise makes sense to merge.

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2273780176)
reviewed through https://github.com/bitcoin/bitcoin/pull/30286/commits/06ea1016ff40ebcf3e668bdb993a0747904cb40f