💬 sipa commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707053089)
@fjahr I guess because I feel it's hard to make concrete promises about something that far out. If we *know* we will remove testnet3 in v30, we should just deprecate it in v28 already (including a `-deprecatedrpc` requirement), rather than saying it will be deprecated. If not, the best we can do is announce an intent that it will be removed in the future.

💬 instagibbs commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2273546756)
> (I want to think a bit more on the potential benefits of https://github.com/bitcoin/bitcoin/pull/30572 before voicing an opinion).

💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707056503)
> Ok, unless I have to re-touch here I will make it consistent in the follow-up where I add the release notes.
>
> > I’m fine with deprecating Testnet3, but I would suggest that the option should only be removed from Bitcoin Core with v29.
>
> That's my intention, since we are adding Testnet4 for v28 the next version I am referring to is v29. Or were you concerned with removing it from master right after v28 is tagged? I would be fine to aim for a removal with v30 instead giving ~1 year of
...

💬 maflcko commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2273559031)
> > it correctly says currently: "Make sure all headers are syncing"
>
> That's not correct in the event of a large fork, where this message would be shown forever.

Correct. However, it seems unrelated to the metadata discussion. At least I fail to see how adding the block height to the metadata in this case helps, when the height is already hardcoded in m_assumeutxo_data. Even if it wasn't, the error message can (and should be) improved for this case, regardless of the metadata contents.
...

💬 ryanofsky commented on issue "RFC: ArgsManager type and range checking":
(https://github.com/bitcoin/bitcoin/issues/22978#issuecomment-2273576739)
Thanks for taking a look!

> I don't think it's trivial to make tinyformat work at compile-time?

Right, `Default<DEFAULT_UPNP>` just provides a default help string format argument. If you want to provide other format arguments you can register with:

```c++
UpnpSetting::Register(args, HelpArgs(help1, help2, DEFAULT_UPNP));
```

instead of:

```c++
UpnpSetting::Register(args);
```

but only a few settings should need this for more complicated formatting.

> Specifying an option
...

💬 Sjors commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2273583932)
> I fail to see how adding the block height to the metadata in this case helps, when the height is already hardcoded in m_assumeutxo_data.

When the user loads a recent snapshot into an older version of Bitcoin Core the height won't be in `m_assumeutxo_data`. That's why we should hold on to the metadata (even if we need to be careful on how to render the error).

💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707092262)
Yes, but not here as done before.

before:
```cpp
// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check.
const uint256 g_block_hash{uint256S("000000002c05cc2e78923c34df87fd108b22221ac6076c18f3ade378a4d915e9")};
uint32_t g_bits{0x1d00ffff};
```

anyway, feel free to ignore it.

💬 andrewtoth commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2273587869)
See also https://github.com/alfred-hodler/pushtx/ which sends unsolicited TXs.

💬 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.