💬 maflcko commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605878784)
The dependency version is picked at install time, and the previous section is about installing the dependencies, so it seems more suitable.

But just a nit, either is fine.

💬 Geremia commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2118963106)
@maflcko

> The debug log says "Shutdown: done", but you claim that "It connects to many peers", indicating that the program is still running?

When I restart bitcoin-qt, it should have asked to reindex, but instead it connects to peers and tries to sync (but stalls):


The debug log doesn't show any more corrupt DB errors than the one I pasted above.

💬 AngusP commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#issuecomment-2118985433)
tACK 8950053636cb38ed85fe2d58b53e5d0acb35c390

I did wonder whether `maxorphantx` was zeroed or otherwise not the default `100` for functests which would perhaps explain it being set here, but I can't see anything to indicate that being the case.

👍 AngusP approved a pull request: "test: remove unneeded `-maxorphantx=1000` settings"
(https://github.com/bitcoin/bitcoin/pull/30133#pullrequestreview-2064941827)
tACK 8950053636cb38ed85fe2d58b53e5d0acb35c390

I did wonder whether `maxorphantx` was zeroed or otherwise not the default `100` for functests which would perhaps explain it being set here, but I can't see anything to indicate that being the case.

💬 AngusP commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#discussion_r1605886064)
nit:
```python
self.extra_args = [
[],
...
```

leaving

```python
self.extra_args = [
[
],
...
```

is a bit weird

💬 edilmedeiros commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605887608)
> Would word it as "Code needs to adhere to the C++20 standard.".

I second this.

💬 Geremia commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2118987509)
I also noticed that from `2024-05-18T04:48:21Z` to `2024-05-18T04:48:35Z` (14 second duration) I got 30 messages in `debug.log` that say:
```
2024-05-18T04:48:21Z UpdateTip: new best=00000000000000000003483012d42cbcd1ebe486364e46a56b8800fadfd43b74 height=843741 version=0x20f12000 log2_work=94.927416 tx=1005741855 date='2024-05-16T18:51:28Z' progress=0.999244 cache=1.4MiB(11882txo) warning='Miner violated version bit protocol'
2024-05-18T04:48:22Z UpdateTip: new best=0000000000000000000014c2b9
...

💬 edilmedeiros commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#discussion_r1605890277)
I don't see this as a problem per se. [test/functional/feature_rbf.py](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754ba8/test/functional/feature_rbf.py#L38) already had the same pattern.

💬 edilmedeiros commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#issuecomment-2118991469)
Tested ACK 8950053636cb38ed85fe2d58b53e5d0acb35c390

The changes don't break the tests.
I took some time to think if the removed params would be essential, but can't see how.

🤔 AngusP reviewed a pull request: "contrib/signet/miner: increase miner search space"
(https://github.com/bitcoin/bitcoin/pull/30130#pullrequestreview-2064943454)
Approach ACK, one suggestion to explicitly check for the exhaustion case and a couple of nits

💬 AngusP commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1605890224)
The `subprocess.CalledProcessError` could contain the stderr&out and the return code -- given there are a couple of other cases where `bitcoin-util grind` can fail, it might be good to check we actually hit an exhaustion rather than the block header decode issue to prevent many retries when something else is wrong (even if in a rare/"should never happen" case)?

```suggestion
if e.returncode == 1 and "Could not satisfy difficulty target" in e.stderr.decode("utf-8"):

...

💬 AngusP commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1605888170)
nit: given this is a debug log message and you're "expecting" this exception, I'm not sure it makes sense to include the word `exception`? Implies something went wrong.

```suggestion
logging.debug('grind subprocess: {}'.format(e))
```

nit-ier: could use a shorter `f` string:

```suggestion
logging.debug(f'grind subprocess: {e}')
```

💬 AngusP commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#discussion_r1605890828)
Fair, it's a stylistic nit so just an opinion, can be disregarded

💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605892537)
Worth adding a note to the comment?

```python
# Use subversion as peer id -- test nodes have their number appended to the UA
```

🤔 AngusP reviewed a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2064946909)
Approach ACK, some nits/questions

💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605892564)
Worth adding a note to the comment?

```python
# Use subversion as peer id. Test nodes have their node number appended to the UA
```

💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605893483)
This comment now doesn't really make sense to me?

Maybe
```suggestion
# wait until peers have completed a version handshake and so match the
# expected connection subversion
```

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605903225)
> This comment now doesn't really make sense to me?

Why? the procedure behavior is the same.

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605903301)
> Worth adding a note to the comment?
>
> ```python
> # Use subversion as peer id. Test nodes have their node number appended to the UA
> ```

sure. Pushed.

💬 rustyrussell commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605921836)
Sure, wasn't sure this was the place, but is there somewhere better? Seems like all the regulars "know" this, so putting it where a beginner might look is helpful.

Was thinking the version info is useful for backporting, but that's a limited window and is already in the commit message if people want.