💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695461472)
Thanks for taking a look!
> Seems fine to add this, though it also takes up a line for information
Right, when calling `-netinfo` without a details level (without the peers list).
I began by adding the services in the versions line, in the same format as in the peers list:
```
Bitcoin Core client v28.99.0 - server 70016/Satoshi:28.99.0/ - services nbwcl2
<-> type net serv v mping ping send recv txn blk hb addrp addrl age asmap id version
in onion nwl2
...
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695461472)
Thanks for taking a look!
> Seems fine to add this, though it also takes up a line for information
Right, when calling `-netinfo` without a details level (without the peers list).
I began by adding the services in the versions line, in the same format as in the peers list:
```
Bitcoin Core client v28.99.0 - server 70016/Satoshi:28.99.0/ - services nbwcl2
<-> type net serv v mping ping send recv txn blk hb addrp addrl age asmap id version
in onion nwl2
...
💬 glozow commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2695473173)
Thank you @laanwj! Would you mind adding it to the release notes draft actually, instead of opening a PR? https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2695473173)
Thank you @laanwj! Would you mind adding it to the release notes draft actually, instead of opening a PR? https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft
💬 mzumsande commented on issue "compact blocks in IBD resets m_stalling_since":
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695499701)
I don't think this would work with blocks below the minchainwork threshold, we should abort [here](https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/src/net_processing.cpp#L4309) -
so large parts of IBD shouldn't be affected. We will still be in IBD for a while after reaching that threshold though, depending on how up-to-date the bitcoind release is.
Also, the node schedules the blocks it downloads during IBD not as an answer to received headers messages, but on
...
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695499701)
I don't think this would work with blocks below the minchainwork threshold, we should abort [here](https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/src/net_processing.cpp#L4309) -
so large parts of IBD shouldn't be affected. We will still be in IBD for a while after reaching that threshold though, depending on how up-to-date the bitcoind release is.
Also, the node schedules the blocks it downloads during IBD not as an answer to received headers messages, but on
...
📝 fjahr opened a pull request: "test: Use rpc_deprecated only for testing deprecation"
(https://github.com/bitcoin/bitcoin/pull/31977)
The comment in `functional/rpc_deprecated.py` says "This test should be used to verify correct behaviour of deprecated RPC methods with and without the -deprecatedrpc flags." I think we can get rid of the "with" part since we can assume that every RPC is already tested in at least one other functional test. (I didn't look but I could verify in our coverage if someone has doubts about that.) In order for this test to continue working, the flag will need to be used there. Otherwise this seems to p
...
(https://github.com/bitcoin/bitcoin/pull/31977)
The comment in `functional/rpc_deprecated.py` says "This test should be used to verify correct behaviour of deprecated RPC methods with and without the -deprecatedrpc flags." I think we can get rid of the "with" part since we can assume that every RPC is already tested in at least one other functional test. (I didn't look but I could verify in our coverage if someone has doubts about that.) In order for this test to continue working, the flag will need to be used there. Otherwise this seems to p
...
💬 fjahr commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978196051)
Note that I think it's enough to test the deprecation error there, see #31977
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978196051)
Note that I think it's enough to test the deprecation error there, see #31977
💬 laanwj commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695543354)
> Nevertheless, I could revert that change if you prefer it remain appended to the versions header, WDYT?
i agree an alphabet soup like "nbwcl2" does not say anything without a legend what every character means.
To be clear, i'm not against adding it, it's only one line, it was more of a general concern about the direction.
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695543354)
> Nevertheless, I could revert that change if you prefer it remain appended to the versions header, WDYT?
i agree an alphabet soup like "nbwcl2" does not say anything without a legend what every character means.
To be clear, i'm not against adding it, it's only one line, it was more of a general concern about the direction.
📝 glozow opened a pull request: "kernel: pre-29.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/31978)
Update chainparams and headerssync config for v29.0 (see [release process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off)).
(https://github.com/bitcoin/bitcoin/pull/31978)
Update chainparams and headerssync config for v29.0 (see [release process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off)).
💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695553587)
I've updated the pull description to be clearer about it.
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695553587)
I've updated the pull description to be clearer about it.
💬 glozow commented on pull request "kernel: pre-29.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/31978#issuecomment-2695559684)
These are from my nodes except for the mainnet `m_assumed_blockchain_size` which is from @sr-gi (thanks!)
(https://github.com/bitcoin/bitcoin/pull/31978#issuecomment-2695559684)
These are from my nodes except for the mainnet `m_assumed_blockchain_size` which is from @sr-gi (thanks!)
💬 pinheadmz commented on issue "test: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=32090)":
(https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2695593851)
I might be seeing this today as well: https://gist.github.com/pinheadmz/08758d268ec1741afc8c4f181c0e97d2
My branch forks off of master at db36a92c02
(https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2695593851)
I might be seeing this today as well: https://gist.github.com/pinheadmz/08758d268ec1741afc8c4f181c0e97d2
My branch forks off of master at db36a92c02
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2695596738)
I gave it a shot and changed it to this:
"Due to a bug the default block reserved weight for fixed-size block header, transactions count, and coinbase transaction was reserved twice and could not be lowered further. As a result the total reserved weight was always `8,000 WU`, meaning that even when specifying a `-blockmaxweight` higher than the default (even to the max `4,000,000 WU`), the actual block size never exceeded `3,992,000 WU`. The fix consolidates the reservation into a single pla
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2695596738)
I gave it a shot and changed it to this:
"Due to a bug the default block reserved weight for fixed-size block header, transactions count, and coinbase transaction was reserved twice and could not be lowered further. As a result the total reserved weight was always `8,000 WU`, meaning that even when specifying a `-blockmaxweight` higher than the default (even to the max `4,000,000 WU`), the actual block size never exceeded `3,992,000 WU`. The fix consolidates the reservation into a single pla
...
💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-2695598930)
I split 5d6aa13cf047e31005afca919ca9cf181e30905b into smaller commits. If we decide to punt this until after v30 then I'll PR some of these separately. E.g. there's some tests that can use testnet4 instead of testnet3 and perhaps we can drop GUI support earlier.
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-2695598930)
I split 5d6aa13cf047e31005afca919ca9cf181e30905b into smaller commits. If we decide to punt this until after v30 then I'll PR some of these separately. E.g. there's some tests that can use testnet4 instead of testnet3 and perhaps we can drop GUI support earlier.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2695600516)
Thanks for the ping @glozow I will amend the suggestions in the wiki
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2695600516)
Thanks for the ping @glozow I will amend the suggestions in the wiki
💬 davidgumberg commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1978281970)
Note: `WriteSettingsFile()` throws if the `-nosettings` argument is set, but in that case we would never find a value for `upnp` above, and `settings_changed` won't be true, so that edge case is handled well.
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1978281970)
Note: `WriteSettingsFile()` throws if the `-nosettings` argument is set, but in that case we would never find a value for `upnp` above, and `settings_changed` won't be true, so that edge case is handled well.
💬 Crypt-iQ commented on issue "compact blocks in IBD resets m_stalling_since":
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695634708)
Ah, I did not consider the minchainwork.
> So it shouldn't be possible to trigger this by sending header messages, but the two attacking peers could be be randomly selected next to each other.
I could have made that more clear in the description that the attacker needed two peers to be selected in sequence for the `SendMessages` call. That's what I meant by "subsequent" but I think it was unclear.
> dependent on already_in_flight - could we unconditionally skip out here if CanDirectFetch() re
...
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695634708)
Ah, I did not consider the minchainwork.
> So it shouldn't be possible to trigger this by sending header messages, but the two attacking peers could be be randomly selected next to each other.
I could have made that more clear in the description that the attacker needed two peers to be selected in sequence for the `SendMessages` call. That's what I meant by "subsequent" but I think it was unclear.
> dependent on already_in_flight - could we unconditionally skip out here if CanDirectFetch() re
...
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2695648258)
I have dropped the `QualityLevel::NEEDS_SPLIT_OPTIMAL` value, and made the "post-linearize after remove, before split" apply to `NEEDS_SPLIT_ACCEPTABLE` instead, with updated comments about it.
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2695648258)
I have dropped the `QualityLevel::NEEDS_SPLIT_OPTIMAL` value, and made the "post-linearize after remove, before split" apply to `NEEDS_SPLIT_ACCEPTABLE` instead, with updated comments about it.
💬 instagibbs commented on issue "compact blocks in IBD resets m_stalling_since":
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695653952)
That code block appears to be in since the dawn of compact blocks: https://github.com/bitcoin/bitcoin/commit/d25cd3ec4e8#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R5312
I might be missing something of course.
> At least removing the condition doesn't break any existing tests.
Not very comforting :)
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695653952)
That code block appears to be in since the dawn of compact blocks: https://github.com/bitcoin/bitcoin/commit/d25cd3ec4e8#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R5312
I might be missing something of course.
> At least removing the condition doesn't break any existing tests.
Not very comforting :)
💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1978311245)
Yes, i used to have an explicit check around this logic for dynamic settings to be enabled (so that it won't throw), but removed it after a comment by @ryanofsky. It indeed doesn't seem to be necessary because the dynamic settings won't be populated in the first place.
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1978311245)
Yes, i used to have an explicit check around this logic for dynamic settings to be enabled (so that it won't throw), but removed it after a comment by @ryanofsky. It indeed doesn't seem to be necessary because the dynamic settings won't be populated in the first place.
💬 davidgumberg commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1978313107)
Thanks, I missed that discussion. (https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965808053)
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1978313107)
Thanks, I missed that discussion. (https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965808053)
💬 laanwj commented on pull request "delete release note fragments for v29":
(https://github.com/bitcoin/bitcoin/pull/31976#issuecomment-2695681192)
ACK ae92bd8e1b2c144697662ba0358c27f5c1892bb2
(https://github.com/bitcoin/bitcoin/pull/31976#issuecomment-2695681192)
ACK ae92bd8e1b2c144697662ba0358c27f5c1892bb2
💬 davidgumberg commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2695688502)
lightly reviewed code, tested ACK https://github.com/bitcoin/bitcoin/commit/44041ae0eca9d2034b7c2bdef24724809cc35e90
There are a lot of possible combinations of settings in bitcoin.conf, commandline arguments, and GUI settings, I sanity tested the happy path of `upnp=true` in settings.json, and a few cases I thought would be tricky with `-nosettings` and `-upnp` as a cli argument and in bitcoin.conf, and this solution handles them all well, and I can't think of any unhandled cases from review
...
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2695688502)
lightly reviewed code, tested ACK https://github.com/bitcoin/bitcoin/commit/44041ae0eca9d2034b7c2bdef24724809cc35e90
There are a lot of possible combinations of settings in bitcoin.conf, commandline arguments, and GUI settings, I sanity tested the happy path of `upnp=true` in settings.json, and a few cases I thought would be tricky with `-nosettings` and `-upnp` as a cli argument and in bitcoin.conf, and this solution handles them all well, and I can't think of any unhandled cases from review
...