📝 brunoerg opened a pull request: "fuzz: use `ConnmanTestMsg` in `connman`"
(https://github.com/bitcoin/bitcoin/pull/28091)
Fixes #27980
Using `ConnmanTestMsg` we can add nodes and be
more effective fuzzing functions like `DisconnectNode`,
`FindNode`, `GetNodeStats` and other ones.
(https://github.com/bitcoin/bitcoin/pull/28091)
Fixes #27980
Using `ConnmanTestMsg` we can add nodes and be
more effective fuzzing functions like `DisconnectNode`,
`FindNode`, `GetNodeStats` and other ones.
📝 fanquake opened a pull request: "build: document that -Wreturn-type has been fixed upstream (mingw-w64)"
(https://github.com/bitcoin/bitcoin/pull/28092)
`noreturn` attributes have been added to the mingw-w64 headers, https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe, meaning that [from 11.0.0 onwards](https://www.mingw-w64.org/changelog/), you'll no-longer see `-Wreturn-type` warnings when using `assert(false)`.
At the same time, move this out of the `-Werror` check, it's not clear why it was in there, rather than being applied/or-not along with all other cxxflags. A somewhat similar change was proposed in
...
(https://github.com/bitcoin/bitcoin/pull/28092)
`noreturn` attributes have been added to the mingw-w64 headers, https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe, meaning that [from 11.0.0 onwards](https://www.mingw-w64.org/changelog/), you'll no-longer see `-Wreturn-type` warnings when using `assert(false)`.
At the same time, move this out of the `-Werror` check, it's not clear why it was in there, rather than being applied/or-not along with all other cxxflags. A somewhat similar change was proposed in
...
💬 fanquake commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1638262812)
> but I think having a macro for those that find it helpful, can't hurt, can it?
I agree that having a macro could be useful.
Separately, note that upstream, the missing `noreturn` attributes have been added to `assert` in the mingw-w64 headers, https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe, so beginning with [11.0.0](https://www.mingw-w64.org/changelog/), there are no-longer `-Wreturn-type` warnings with `assert(false)` usage. See #28092 where I've
...
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1638262812)
> but I think having a macro for those that find it helpful, can't hurt, can it?
I agree that having a macro could be useful.
Separately, note that upstream, the missing `noreturn` attributes have been added to `assert` in the mingw-w64 headers, https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe, so beginning with [11.0.0](https://www.mingw-w64.org/changelog/), there are no-longer `-Wreturn-type` warnings with `assert(false)` usage. See #28092 where I've
...
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265452083)
I haven't done a careful review of all the places we use `IsInitialBlockDownload()` to confirm this, but my intuition would be that if we're using an assumeutxo snapshot, what we care about is whether our snapshot-based chainstate is caught up or not -- basically by definition, the background chainstate will always be "in initial block download" until it finishes and is no longer in use, so it wouldn't make sense to condition anything on whether it's behind or not.
So yeah I think moving this
...
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265452083)
I haven't done a careful review of all the places we use `IsInitialBlockDownload()` to confirm this, but my intuition would be that if we're using an assumeutxo snapshot, what we care about is whether our snapshot-based chainstate is caught up or not -- basically by definition, the background chainstate will always be "in initial block download" until it finishes and is no longer in use, so it wouldn't make sense to condition anything on whether it's behind or not.
So yeah I think moving this
...
💬 theStack commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265452959)
Sounds like a good idea! Couldn't get that to compile yet unfortunately 🙁 (which is maybe due to my lack of knowledge about templates). I tried with the following patch:
```diff
diff --git a/src/crypto/siphash.cpp b/src/crypto/siphash.cpp
index 2c328d0840..7cbafd4d98 100644
--- a/src/crypto/siphash.cpp
+++ b/src/crypto/siphash.cpp
@@ -45,7 +45,8 @@ CSipHasher& CSipHasher::Write(uint64_t data)
return *this;
}
-CSipHasher& CSipHasher::Write(Span<const unsigned char> data)
+temp
...
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265452959)
Sounds like a good idea! Couldn't get that to compile yet unfortunately 🙁 (which is maybe due to my lack of knowledge about templates). I tried with the following patch:
```diff
diff --git a/src/crypto/siphash.cpp b/src/crypto/siphash.cpp
index 2c328d0840..7cbafd4d98 100644
--- a/src/crypto/siphash.cpp
+++ b/src/crypto/siphash.cpp
@@ -45,7 +45,8 @@ CSipHasher& CSipHasher::Write(uint64_t data)
return *this;
}
-CSipHasher& CSipHasher::Write(Span<const unsigned char> data)
+temp
...
💬 pinheadmz commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#issuecomment-1638272123)
> Additionally, a new dialog is added to the GUI that allows the user to select the change output when they are bumping the transaction fee. This dialog will default select output that the wallet determines to be change, if it could find one.
Not seeing this in this branch.
(https://github.com/bitcoin/bitcoin/pull/26467#issuecomment-1638272123)
> Additionally, a new dialog is added to the GUI that allows the user to select the change output when they are bumping the transaction fee. This dialog will default select output that the wallet determines to be change, if it could find one.
Not seeing this in this branch.
💬 MarcoFalke commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265463653)
Ah, sorry, the `const` prevents a match. You can just drop it. Or alternatively put in the header two overloads of the form:
```cpp
CSipHasher& Write(Span<const unsigned char> data);
CSipHasher& Write(Span<const std::byte> data);
```
And the make one call the other in the cpp file.
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265463653)
Ah, sorry, the `const` prevents a match. You can just drop it. Or alternatively put in the header two overloads of the form:
```cpp
CSipHasher& Write(Span<const unsigned char> data);
CSipHasher& Write(Span<const std::byte> data);
```
And the make one call the other in the cpp file.
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265477633)
We may not have received the snapshot base block, and it's only when we call `ReceivedBlockTransactions` on a block that we set VALID_TRANSACTIONS on the block index entry. So the snapshot base block may not otherwise be considered a block index candidate, but the snapshot base block could be the tip of the snapshot chain so we need to be able to call `TryAddBlockIndexCandidate()`.
I'll push an update that adds a comment explaining this.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265477633)
We may not have received the snapshot base block, and it's only when we call `ReceivedBlockTransactions` on a block that we set VALID_TRANSACTIONS on the block index entry. So the snapshot base block may not otherwise be considered a block index candidate, but the snapshot base block could be the tip of the snapshot chain so we need to be able to call `TryAddBlockIndexCandidate()`.
I'll push an update that adds a comment explaining this.
💬 furszy commented on pull request "test: Bump walletpassphrase timeout to avoid intermittent issue":
(https://github.com/bitcoin/bitcoin/pull/28089#discussion_r1265478128)
> An alternative might be to not use a passphrase after this test
Yeah. Would be even better to create a wallet specific to this test and unload it at the end of it.
> Do you want to submit either as a pull request? Happy to close mine then. Otherwise, I'll leave the feedback for a follow-up, as the goal here is to not change any logic, only the constant.
Will try to do it later today. Shouldn't take me much.
Still, all good if this gets merged before it.
(https://github.com/bitcoin/bitcoin/pull/28089#discussion_r1265478128)
> An alternative might be to not use a passphrase after this test
Yeah. Would be even better to create a wallet specific to this test and unload it at the end of it.
> Do you want to submit either as a pull request? Happy to close mine then. Otherwise, I'll leave the feedback for a follow-up, as the goal here is to not change any logic, only the constant.
Will try to do it later today. Shouldn't take me much.
Still, all good if this gets merged before it.
💬 Crypt-iQ commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1638306663)
> OK I've pushed a new set of changes which now disconnects nodes synchronously inside of `AttemptToEvictConnection`.
>
> @Crypt-iQ I'd be curious if you still see these new changes as resolving the issue in #27843? I havent' gotten your test patch working to my satisfaction yet (or at least, I don't see positive eviction candidate selection during it so it wouldn't overflow `nMaxInbound` even without these changes).
Sorry I have been a bit short on time recently, but let me know when to t
...
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1638306663)
> OK I've pushed a new set of changes which now disconnects nodes synchronously inside of `AttemptToEvictConnection`.
>
> @Crypt-iQ I'd be curious if you still see these new changes as resolving the issue in #27843? I havent' gotten your test patch working to my satisfaction yet (or at least, I don't see positive eviction candidate selection during it so it wouldn't overflow `nMaxInbound` even without these changes).
Sorry I have been a bit short on time recently, but let me know when to t
...
💬 sipa commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265489980)
While I don't obect to having this particular function templatized, I don't think it's a scalable approach to introduce dual char/byte functions all over the codebase, just because not everything is using bytes.
IMO, the goal should be to *only* have a byte interface here; The options for that are:
* Just ~byte~ bite the bullet now, and introduce `MakeByteSpan` in all call sites.
* Stick with a `Span<const unsigned char>` interface for now, and revisit at some point in the future when more
...
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265489980)
While I don't obect to having this particular function templatized, I don't think it's a scalable approach to introduce dual char/byte functions all over the codebase, just because not everything is using bytes.
IMO, the goal should be to *only* have a byte interface here; The options for that are:
* Just ~byte~ bite the bullet now, and introduce `MakeByteSpan` in all call sites.
* Stick with a `Span<const unsigned char>` interface for now, and revisit at some point in the future when more
...
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265513530)
The idea here is that this check is part of a heuristic that we use when we receive a block that was not requested. For anti-DoS reasons, back in #5875 we introduced logic to generally not process unrequested blocks, but we left in an optimization that was intended to not throw away blocks that would immediately advance our tip, in case we had some peer (like Matt's old fast-relay-network, if I remember right) that was sending such blocks (see eg comment here https://github.com/bitcoin/bitcoin/
...
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265513530)
The idea here is that this check is part of a heuristic that we use when we receive a block that was not requested. For anti-DoS reasons, back in #5875 we introduced logic to generally not process unrequested blocks, but we left in an optimization that was intended to not throw away blocks that would immediately advance our tip, in case we had some peer (like Matt's old fast-relay-network, if I remember right) that was sending such blocks (see eg comment here https://github.com/bitcoin/bitcoin/
...
💬 MarcoFalke commented on pull request "fuzz: use `ConnmanTestMsg` in `connman`":
(https://github.com/bitcoin/bitcoin/pull/28091#issuecomment-1638355685)
lgtm ACK d9bd559638b453b8c6e03e34005ad5c52099b6bf
(https://github.com/bitcoin/bitcoin/pull/28091#issuecomment-1638355685)
lgtm ACK d9bd559638b453b8c6e03e34005ad5c52099b6bf
💬 MarcoFalke commented on pull request "test: fix `feature_addrman.py` on big-endian systems":
(https://github.com/bitcoin/bitcoin/pull/27529#issuecomment-1638360511)
See https://github.com/bitcoin/bitcoin/pull/28087
(https://github.com/bitcoin/bitcoin/pull/27529#issuecomment-1638360511)
See https://github.com/bitcoin/bitcoin/pull/28087
💬 fanquake commented on pull request "build: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1638363452)
Remembered why we can't make this change; because the current code was added to our buildsystem as a hack to fix the Windows CI under `-Werror`, rather than just suppressing the incorrect warning... Took my own advice from 25972 and deleted the check entirely, adding `-Wno-return-type` to the CI, where it can be dropped once we are using a fixed version of the headers.
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1638363452)
Remembered why we can't make this change; because the current code was added to our buildsystem as a hack to fix the Windows CI under `-Werror`, rather than just suppressing the incorrect warning... Took my own advice from 25972 and deleted the check entirely, adding `-Wno-return-type` to the CI, where it can be dropped once we are using a fixed version of the headers.
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1638374417)
Just pushed a new branch incorporating feedback from all of @ryanofsky's prior review. Regarding https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1251289117, I decided that it made the most sense to invoke `ActivateBestChain()` on both chainstates in `LoadExternalBlockFile` if pruning was enabled, even though I think it'll take some more work to figure out how pruning should function in the event that we're in the middle of assumeutxo-initiated background validation.
Regarding the c
...
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1638374417)
Just pushed a new branch incorporating feedback from all of @ryanofsky's prior review. Regarding https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1251289117, I decided that it made the most sense to invoke `ActivateBestChain()` on both chainstates in `LoadExternalBlockFile` if pruning was enabled, even though I think it'll take some more work to figure out how pruning should function in the event that we're in the middle of assumeutxo-initiated background validation.
Regarding the c
...
👍 pinheadmz approved a pull request: "bumpfee: Allow the user to choose which output is change"
(https://github.com/bitcoin/bitcoin/pull/26467#pullrequestreview-1532950806)
re-ACK e8c31f135c6e9a5f57325dbf4feceafd384f7762
Reviewed all code changes, ran tests locally. I played with the feature in regtest with wallet TXs as well as PSBTs. Made sure invalid input threw errors and expected output was reduced by fee bump. Found one nit below. Maybe remove GUI from OP description? Is that being added in a bitcoin-core/gui PR ?
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK e8c31f135c6e9a5f57325dbf4feceafd38
...
(https://github.com/bitcoin/bitcoin/pull/26467#pullrequestreview-1532950806)
re-ACK e8c31f135c6e9a5f57325dbf4feceafd384f7762
Reviewed all code changes, ran tests locally. I played with the feature in regtest with wallet TXs as well as PSBTs. Made sure invalid input threw errors and expected output was reduced by fee bump. Found one nit below. Maybe remove GUI from OP description? Is that being added in a bitcoin-core/gui PR ?
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK e8c31f135c6e9a5f57325dbf4feceafd38
...
💬 pinheadmz commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1265458854)
nit: extra space?
```suggestion
if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {
```
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1265458854)
nit: extra space?
```suggestion
if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {
```
💬 achow101 commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#issuecomment-1638376415)
> > Additionally, a new dialog is added to the GUI that allows the user to select the change output when they are bumping the transaction fee. This dialog will default select output that the wallet determines to be change, if it could find one.
>
> Not seeing this in this branch.
That was moved to https://github.com/bitcoin-core/gui/pull/700. Dropped that from the PR description.
(https://github.com/bitcoin/bitcoin/pull/26467#issuecomment-1638376415)
> > Additionally, a new dialog is added to the GUI that allows the user to select the change output when they are bumping the transaction fee. This dialog will default select output that the wallet determines to be change, if it could find one.
>
> Not seeing this in this branch.
That was moved to https://github.com/bitcoin-core/gui/pull/700. Dropped that from the PR description.
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265542867)
For context, the issue was originally reported in #17890 and was fixed in #17994. It took me a while to understand exactly what this logic was for and how it worked, so a test to ensure we don't introduce a regression would be helpful!
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265542867)
For context, the issue was originally reported in #17890 and was fixed in #17994. It took me a while to understand exactly what this logic was for and how it worked, so a test to ensure we don't introduce a regression would be helpful!