💬 Willrok91 commented on issue "Generalized fee bumping":
(https://github.com/bitcoin-core/gui/issues/822#issuecomment-2641233162)
bc1q7ppm2rxrw73heugq334msxcmxu5xfr7eh37ne7
(https://github.com/bitcoin-core/gui/issues/822#issuecomment-2641233162)
bc1q7ppm2rxrw73heugq334msxcmxu5xfr7eh37ne7
💬 Willrok91 commented on issue "Send: ability to (re)view automatically selected coins ":
(https://github.com/bitcoin-core/gui/issues/778#issuecomment-2641236859)
bc1q7ppm2rxrw73heugq334msxcmxu5xfr7eh37ne7
(https://github.com/bitcoin-core/gui/issues/778#issuecomment-2641236859)
bc1q7ppm2rxrw73heugq334msxcmxu5xfr7eh37ne7
💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2641303013)
> It might be good to also adjust the title of this test for CoinGrinder to "3) Test selection when some selections with sufficient funds would exceed the maximum allowed weight"
I find the use of "selection" twice in the sentence doesn't add much clarity. Now with the added assertions, I feel like the tittle fits the test however I'm open to a better title.
> Not really. The weight-optimality of CoinGrinder solutions is shown per the fuzz target coin_grinder_is_optimal.
Interesting.
...
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2641303013)
> It might be good to also adjust the title of this test for CoinGrinder to "3) Test selection when some selections with sufficient funds would exceed the maximum allowed weight"
I find the use of "selection" twice in the sentence doesn't add much clarity. Now with the added assertions, I feel like the tittle fits the test however I'm open to a better title.
> Not really. The weight-optimality of CoinGrinder solutions is shown per the fuzz target coin_grinder_is_optimal.
Interesting.
...
💬 jarolrod commented on pull request "Prepare "Open Transifex translations for v29.0" release step":
(https://github.com/bitcoin/bitcoin/pull/31809#issuecomment-2641784769)
post merge ack
(https://github.com/bitcoin/bitcoin/pull/31809#issuecomment-2641784769)
post merge ack
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945972101)
Done.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945972101)
Done.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945976586)
No, we do not want to remove entries from `m_listen_permissions` or from `m_listen` when disconnecting a single peer. The entries in those two maps are per listening address. E.g. if we listen on `1.1.1.1:8333` and designate that every peer that connects to that address gets `permissions1` and listen on `2.2.2.2:8333` and peers that connect to that address get `permissions2`. This stays unchanged as peers connect and disconnect. We would want to remove entries from those maps if we stop listenin
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945976586)
No, we do not want to remove entries from `m_listen_permissions` or from `m_listen` when disconnecting a single peer. The entries in those two maps are per listening address. E.g. if we listen on `1.1.1.1:8333` and designate that every peer that connects to that address gets `permissions1` and listen on `2.2.2.2:8333` and peers that connect to that address get `permissions2`. This stays unchanged as peers connect and disconnect. We would want to remove entries from those maps if we stop listenin
...
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945983707)
`addr` is our listening address. We can't listen on the same address two times:
If I put this in my `bitcoind.conf`:
```
bind=127.0.0.1:20001
bind=127.0.0.1:20002
bind=127.0.0.1:20003
bind=127.0.0.1:20003
```
I get:
```
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20001
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20002
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20003
2025-02-07T05:39:56Z [net:error] Cannot
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945983707)
`addr` is our listening address. We can't listen on the same address two times:
If I put this in my `bitcoind.conf`:
```
bind=127.0.0.1:20001
bind=127.0.0.1:20002
bind=127.0.0.1:20003
bind=127.0.0.1:20003
```
I get:
```
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20001
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20002
2025-02-07T05:39:56Z [net:info] Bound to and listening on 127.0.0.1:20003
2025-02-07T05:39:56Z [net:error] Cannot
...
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945989789)
The proposed snippet is more elegant and performant, I like it. It relies, however, on `None` being `0` and will break in a subtle way if that is changed. I will leave it as it is. This is executed once per newly accepted connection which is not super-often and the performance gain would be negligible.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945989789)
The proposed snippet is more elegant and performant, I like it. It relies, however, on `None` being `0` and will break in a subtle way if that is changed. I will leave it as it is. This is executed once per newly accepted connection which is not super-often and the performance gain would be negligible.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945991471)
Agree, will leave it as it is.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945991471)
Agree, will leave it as it is.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946000478)
This method was only dealing with listening sockets, so the name `CloseSockets()` with a comment "Close all sockets" was confusing. Renamed to `StopListening()`.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946000478)
This method was only dealing with listening sockets, so the name `CloseSockets()` with a comment "Close all sockets" was confusing. Renamed to `StopListening()`.
💬 maflcko commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2642142041)
lgtm ACK 517d30b097d17f86d40beff679f62287776c459a
I haven't tested this (I am using a modified script locally anyway), but the code changes look harmless and reasonable.
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2642142041)
lgtm ACK 517d30b097d17f86d40beff679f62287776c459a
I haven't tested this (I am using a modified script locally anyway), but the code changes look harmless and reasonable.
💬 theStack commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1946078456)
Thanks for the reviews. The build path can now specified via the `BUILDDIR` environment variable (set to "build" relative to the top-level-dir by default) and the script can be called from anywhere within the repository.
@maflcko
> This would also allow to remove mktemp and just use the build dir.
Done.
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1946078456)
Thanks for the reviews. The build path can now specified via the `BUILDDIR` environment variable (set to "build" relative to the top-level-dir by default) and the script can be called from anywhere within the repository.
@maflcko
> This would also allow to remove mktemp and just use the build dir.
Done.
💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1946082845)
Right, but shouldn't `getaddressinfo` hide the field entirely then? (obviously not in this PR)
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1946082845)
Right, but shouldn't `getaddressinfo` hide the field entirely then? (obviously not in this PR)
💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1946126512)
Done. Also renamed the variable to `_crc32_src`.
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1946126512)
Done. Also renamed the variable to `_crc32_src`.
💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1946127776)
I just reused the existing style and just renamed the target and the keyword. Reformatting that line has more ripple effects in the lines that follow.
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1946127776)
I just reused the existing style and just renamed the target and the keyword. Reformatting that line has more ripple effects in the lines that follow.
💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2642221055)
> I think that APPENDing to the COMPILE_OPTIONS property should be preferred over overriding it.
The command line flags that are passed to the compiler are constructed by several variables and properties that are global, target specific, and file specific. Since this is the only file specific option, APPENDing has no different effect as setting.
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2642221055)
> I think that APPENDing to the COMPILE_OPTIONS property should be preferred over overriding it.
The command line flags that are passed to the compiler are constructed by several variables and properties that are global, target specific, and file specific. Since this is the only file specific option, APPENDing has no different effect as setting.
💬 Sjors commented on issue "Support validating a PoW-free block over over RPC":
(https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2642228270)
#31564 adds this to the Mining interface. It's not needed for the Stratum v2 Template Provider, so lower priority for me.
Additionally it introduces a `checkblock` RPC, which seems easier to use and easier to expand later.
Finally it can verify weak block proof-of-work.
(https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2642228270)
#31564 adds this to the Mining interface. It's not needed for the Stratum v2 Template Provider, so lower priority for me.
Additionally it introduces a `checkblock` RPC, which seems easier to use and easier to expand later.
Finally it can verify weak block proof-of-work.
💬 Sjors commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#discussion_r1946163054)
Actually it's not needed. This code is for the proposal mode, where we verify a block given to us. It's not _our_ node error if this block is invalid.
When we generate a block template ourselves through `getblocktemplate` or `createNewBlock()`, it goes through `BlockAssembler::CreateNewBlock()` which by default runs `TestBlockValidity` at the end, and throws if this return an error. No need to log if we're already crashing with `std::runtime_error`.
(https://github.com/bitcoin/bitcoin/pull/31564#discussion_r1946163054)
Actually it's not needed. This code is for the proposal mode, where we verify a block given to us. It's not _our_ node error if this block is invalid.
When we generate a block template ourselves through `getblocktemplate` or `createNewBlock()`, it goes through `BlockAssembler::CreateNewBlock()` which by default runs `TestBlockValidity` at the end, and throws if this return an error. No need to log if we're already crashing with `std::runtime_error`.
💬 l0rinc commented on pull request "doc: update translation generation cmake example":
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1946169077)
This PR was meant to fix the example only, I don't have enough context to update the docs as well.
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1946169077)
This PR was meant to fix the example only, I don't have enough context to update the docs as well.
💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1946211743)
done
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1946211743)
done