💬 maflcko commented on pull request "test: Remove redundant verack check":
(https://github.com/bitcoin/bitcoin/pull/30252#discussion_r1632619229)
I think the name `fSuccessfullyConnected==false` implies that the connection may not be successfully used (yet), so I think I'll leave it as-is for now.
Also, I think "Will ignore *all* messges received" isn't true, because the node will accept some message types.
What about: "Some message types are blocked from being sent or received before `fSuccessfullyConnected`."?
(https://github.com/bitcoin/bitcoin/pull/30252#discussion_r1632619229)
I think the name `fSuccessfullyConnected==false` implies that the connection may not be successfully used (yet), so I think I'll leave it as-is for now.
Also, I think "Will ignore *all* messges received" isn't true, because the node will accept some message types.
What about: "Some message types are blocked from being sent or received before `fSuccessfullyConnected`."?
💬 S3RK commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#issuecomment-2157495123)
light code review ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
(https://github.com/bitcoin/bitcoin/pull/29521#issuecomment-2157495123)
light code review ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
💬 S3RK commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632757408)
nit: it makes sense to keep this check before coin selection to return early and avoid wasting time
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632757408)
nit: it makes sense to keep this check before coin selection to return early and avoid wasting time
💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632790468)
Yeah, I had considered this but it would require refactoring `IsDust` in a non-trivial way, since `IsDust` is used outside of the wallet and as such doesn't really know what a `CRecipient` is.
It's definitely possible, since all you really need to check for dust is the serialized size of the output and the value, but doing the refactor here blows up the scope of this PR quite a bit. I think it would be better to handle this in a separate PR, i.e. refactor `IsDust` so it can be used for both `
...
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632790468)
Yeah, I had considered this but it would require refactoring `IsDust` in a non-trivial way, since `IsDust` is used outside of the wallet and as such doesn't really know what a `CRecipient` is.
It's definitely possible, since all you really need to check for dust is the serialized size of the output and the value, but doing the refactor here blows up the scope of this PR quite a bit. I think it would be better to handle this in a separate PR, i.e. refactor `IsDust` so it can be used for both `
...
👍 hebasto approved a pull request: "build: Remove --enable-gprof"
(https://github.com/bitcoin/bitcoin/pull/30257#pullrequestreview-2106998558)
ACK fa780e1c25e8e98253e32d93db65f78a0092433f, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/30257#pullrequestreview-2106998558)
ACK fa780e1c25e8e98253e32d93db65f78a0092433f, I have reviewed the code and it looks OK.
🚀 fanquake merged a pull request: "refactor: performance-for-range-copy in psbt.h"
(https://github.com/bitcoin/bitcoin/pull/30253)
(https://github.com/bitcoin/bitcoin/pull/30253)
💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632823304)
Actually, I just realized I was over complicating this: you don't need to refactor `IsDust`, you just need to wrap it with an `IsDust` function that turns a `CRecipient` into a `CTxOut`, same as `GetSerializeSizeForRecipient`. Will update.
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632823304)
Actually, I just realized I was over complicating this: you don't need to refactor `IsDust`, you just need to wrap it with an `IsDust` function that turns a `CRecipient` into a `CTxOut`, same as `GetSerializeSizeForRecipient`. Will update.
👍 fanquake approved a pull request: "build: warn on self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30235#pullrequestreview-2107065005)
ACK 15796d4b61342f75548b20a18c670ed21d102ba8 - not a huge fan of inline pragma usage, but this seems fine, given it's to work around an already-fixed compiler bug, and we'll only be carrying it for a shortish time in any case.
(https://github.com/bitcoin/bitcoin/pull/30235#pullrequestreview-2107065005)
ACK 15796d4b61342f75548b20a18c670ed21d102ba8 - not a huge fan of inline pragma usage, but this seems fine, given it's to work around an already-fixed compiler bug, and we'll only be carrying it for a shortish time in any case.
🚀 fanquake merged a pull request: "build: warn on self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30235)
(https://github.com/bitcoin/bitcoin/pull/30235)
💬 fanquake commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2157716676)
> @fanquake Would you be ok with carrying this in our subtree? I'm not sure of a better solution.
Yea I think that's fine.
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2157716676)
> @fanquake Would you be ok with carrying this in our subtree? I'm not sure of a better solution.
Yea I think that's fine.
💬 fanquake commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632843604)
Wondering if it's just sporadic, given previous pushes passed, i.e https://cirrus-ci.com/task/5429980706897920, and it's doesn't look like much has changed upstream recently. Will push again soonish.
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632843604)
Wondering if it's just sporadic, given previous pushes passed, i.e https://cirrus-ci.com/task/5429980706897920, and it's doesn't look like much has changed upstream recently. Will push again soonish.
💬 glozow commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-2157725577)
ACK 429ec1aaaa
Thank you for rebasing!
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-2157725577)
ACK 429ec1aaaa
Thank you for rebasing!
💬 maflcko commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632851441)
Yeah, I guess it is similar to the intermittent wine errors on master that are already happening too frequent
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632851441)
Yeah, I guess it is similar to the intermittent wine errors on master that are already happening too frequent
🤔 glozow reviewed a pull request: "test: expand LimitOrphan and EraseForPeer coverage"
(https://github.com/bitcoin/bitcoin/pull/30082#pullrequestreview-2107113637)
ACK eaf4de028de8fa13227b6089785889f1c6c15b4d
(https://github.com/bitcoin/bitcoin/pull/30082#pullrequestreview-2107113637)
ACK eaf4de028de8fa13227b6089785889f1c6c15b4d
💬 hebasto commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2157737186)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/227.
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2157737186)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/227.
💬 fanquake commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2157739910)
> Any reason this is still in draft? I agree it seems useful do enable before the CMake switch.
Rebased, and fixed the conflict. Added another change for the Win64 CI. We'll probably need to fixup the failing ARM and previous releases job upstream:
```bash
In file included from minisketch/src/fields/generic_common_impl.h:12,
from minisketch/src/fields/generic_4bytes.cpp:12:
minisketch/src/fields/../int_utils.h:162:7: error: "HAVE_CLZ" is not defined, evaluates to 0 [-Werr
...
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2157739910)
> Any reason this is still in draft? I agree it seems useful do enable before the CMake switch.
Rebased, and fixed the conflict. Added another change for the Win64 CI. We'll probably need to fixup the failing ARM and previous releases job upstream:
```bash
In file included from minisketch/src/fields/generic_common_impl.h:12,
from minisketch/src/fields/generic_4bytes.cpp:12:
minisketch/src/fields/../int_utils.h:162:7: error: "HAVE_CLZ" is not defined, evaluates to 0 [-Werr
...
💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2157745629)
Re https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2106239011
> is this tested anywhere?
No, and I'm not sure it should be given we don't support this. Maybe we can add a comment and assert that just wiping the block index db is not supported for now?
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2157745629)
Re https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2106239011
> is this tested anywhere?
No, and I'm not sure it should be given we don't support this. Maybe we can add a comment and assert that just wiping the block index db is not supported for now?
💬 hebasto commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2157769746)
> This was the multiprocess job failure:
cc @ryanofsky
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2157769746)
> This was the multiprocess job failure:
cc @ryanofsky
💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1632112358)
(Would be nice to add something like
```
# Node3, -bind=... and -listenonion.
self.expected.append(
[
[f"-bind=127.0.0.1:{port}", "-listenonion"],
[(loopback_ipv4, port)]
],
)
port += 1
```
But would then also be nice to verify whether a port was being used for onion traffic or not, to distinguish Node3 from Node2, which I don't know how to do).
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1632112358)
(Would be nice to add something like
```
# Node3, -bind=... and -listenonion.
self.expected.append(
[
[f"-bind=127.0.0.1:{port}", "-listenonion"],
[(loopback_ipv4, port)]
],
)
port += 1
```
But would then also be nice to verify whether a port was being used for onion traffic or not, to distinguish Node3 from Node2, which I don't know how to do).
💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1632107735)
Thanks for taking my suggestion in https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1607299718! To increase readability one can also use the named loop-variable here, sorry I didn't mention it before:
```suggestion
assert_equal(binds, set(expected_services))
```
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1632107735)
Thanks for taking my suggestion in https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1607299718! To increase readability one can also use the named loop-variable here, sorry I didn't mention it before:
```suggestion
assert_equal(binds, set(expected_services))
```