💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241352665)
I think we can constify the value, e.g `PER_INPUT_MAX_ANNEX_SIZE`, generally this is unclear if we would like a per-input annex limit and a max transaction all annexes limits, e.g `MAX_ANNEX_BUDGET`.
Splitting the limit enables flow where inputs can be combined in a non-deterministic order, e.g BIP174’s [combiner](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki#combiner), and reject in function of “witness weight slots” that can be distributed by such combiner, which makes sens
...
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241352665)
I think we can constify the value, e.g `PER_INPUT_MAX_ANNEX_SIZE`, generally this is unclear if we would like a per-input annex limit and a max transaction all annexes limits, e.g `MAX_ANNEX_BUDGET`.
Splitting the limit enables flow where inputs can be combined in a non-deterministic order, e.g BIP174’s [combiner](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki#combiner), and reject in function of “witness weight slots” that can be distributed by such combiner, which makes sens
...
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241355849)
Should we introduce a limit on the number of inputs supported in a transaction opted-in annex ? To make it easier to reason on worst-case scenario from a second-layer perspective in case of multi-party flow e.g dual-funding. This number of inputs could be tied to the transaction’s `nVersion` field.
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241355849)
Should we introduce a limit on the number of inputs supported in a transaction opted-in annex ? To make it easier to reason on worst-case scenario from a second-layer perspective in case of multi-party flow e.g dual-funding. This number of inputs could be tied to the transaction’s `nVersion` field.
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241344893)
I think here we might prefer to _not_ discount the 0x00 signaling byte from the 265 byte anti-annex-inflation-attacks limit, otherwise it sounds to me we’re introducing a coupling effect between the max witness size one can sign for and the tag signaling byte format itself. Such tag signaling byte format might acquire consensus semantic in the future, and I think we would prefer to reserve the evolvability in function of consensus changes here.
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241344893)
I think here we might prefer to _not_ discount the 0x00 signaling byte from the 265 byte anti-annex-inflation-attacks limit, otherwise it sounds to me we’re introducing a coupling effect between the max witness size one can sign for and the tag signaling byte format itself. Such tag signaling byte format might acquire consensus semantic in the future, and I think we would prefer to reserve the evolvability in function of consensus changes here.
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241358413)
If we can move the annex policy-only code in its own `src/policy/annex` like we have at the image of `src/policy/packages.h` for code modularity. I think in the discussion of [#1381](https://github.com/bitcoin/bips/pull/1381) there has been question to reuse the annex in the future for [Grafroot](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-February/015700.html).
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241358413)
If we can move the annex policy-only code in its own `src/policy/annex` like we have at the image of `src/policy/packages.h` for code modularity. I think in the discussion of [#1381](https://github.com/bitcoin/bips/pull/1381) there has been question to reuse the annex in the future for [Grafroot](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-February/015700.html).
💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1241361274)
nit: unrelated whitespace change
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1241361274)
nit: unrelated whitespace change
💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1241362625)
I think `addrConnect` can't be used here, because it can be empty if `pszDest` is set (that's why the CI fails too). You could use `pnode->addr` instead, which is set from either `addrConnect` or `pszDest` in `ConnectNode()`!
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1241362625)
I think `addrConnect` can't be used here, because it can be empty if `pszDest` is set (that's why the CI fails too). You could use `pnode->addr` instead, which is set from either `addrConnect` or `pszDest` in `ConnectNode()`!
💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1241361033)
here and in `test/util/net.h` - this is still using `ConnectedThroughNetwork` (see previous comment).
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1241361033)
here and in `test/util/net.h` - this is still using `ConnectedThroughNetwork` (see previous comment).
📝 Jezebel5134567 opened a pull request: "Create bitcoinhh"
(https://github.com/bitcoin/bitcoin/pull/27970)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/27970)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 Jezebel5134567 commented on pull request "Create bitcoinhh":
(https://github.com/bitcoin/bitcoin/pull/27970#issuecomment-1606398607)
btc
(https://github.com/bitcoin/bitcoin/pull/27970#issuecomment-1606398607)
btc
💬 ariard commented on pull request "p2p: Advertise `NODE_FULL_RBF` and connect to 4 outbound full-rbf peers if `-mempoolfullrbf` is set":
(https://github.com/bitcoin/bitcoin/pull/25600#issuecomment-1606414648)
In the context of #27926, I think we might have to reconsider automatic transaction-relay preferential peering on the table, therefore allowing full-node operators to opt-in with `annexrelay=true`.
Beyond, such use-case, I think the preferential peering could be abstracted like we have for consensus-deployment code in `/consensus/params.h` and `DeploymentHeight()` in src/validation.cpp`. This could be re-used not only for `mempoolfullrbf` and future replacemnt logic upgrade, but also for `nVe
...
(https://github.com/bitcoin/bitcoin/pull/25600#issuecomment-1606414648)
In the context of #27926, I think we might have to reconsider automatic transaction-relay preferential peering on the table, therefore allowing full-node operators to opt-in with `annexrelay=true`.
Beyond, such use-case, I think the preferential peering could be abstracted like we have for consensus-deployment code in `/consensus/params.h` and `DeploymentHeight()` in src/validation.cpp`. This could be re-used not only for `mempoolfullrbf` and future replacemnt logic upgrade, but also for `nVe
...
⚠️ tansanDOTeth opened an issue: "Keep getting errors after a while of syncing"
(https://github.com/bitcoin/bitcoin/issues/27972)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Spent the last few weeks trying to sync a full node and keep getting a wild error after making lots of progress. I had to restart at least 3 times, but decided to finally report it here.
> 2023-06-26T04:05:04Z LevelDB read failure: Corruption: block checksum mismatch: /Volumes/Crucial X8/BTC Blockchain Data/chainstate/031566.ldb
2023-06-26T04:05:04Z Fatal LevelDB error: Corruption: blo
...
(https://github.com/bitcoin/bitcoin/issues/27972)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Spent the last few weeks trying to sync a full node and keep getting a wild error after making lots of progress. I had to restart at least 3 times, but decided to finally report it here.
> 2023-06-26T04:05:04Z LevelDB read failure: Corruption: block checksum mismatch: /Volumes/Crucial X8/BTC Blockchain Data/chainstate/031566.ldb
2023-06-26T04:05:04Z Fatal LevelDB error: Corruption: blo
...
✅ achow101 closed a pull request: "Create bitcoinhh"
(https://github.com/bitcoin/bitcoin/pull/27970)
(https://github.com/bitcoin/bitcoin/pull/27970)
📝 achow101 locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/27970)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/27970)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 tansanDOTeth commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/21724#issuecomment-1606570109)
> I had this checksum LevelDB problem frequently in the last weeks, following this answer [#10897 (comment)](https://github.com/bitcoin/bitcoin/issues/10897#issuecomment-317355977) I managed to solve it, the HD I have been using to keep a copy of bitcoins blockchain is not working properly anymore, copying everything to a new HD and using it completely solve the errors.
I read that comment, but not sure what I have to do run tests against my external. Do you have more instructions written som
...
(https://github.com/bitcoin/bitcoin/issues/21724#issuecomment-1606570109)
> I had this checksum LevelDB problem frequently in the last weeks, following this answer [#10897 (comment)](https://github.com/bitcoin/bitcoin/issues/10897#issuecomment-317355977) I managed to solve it, the HD I have been using to keep a copy of bitcoins blockchain is not working properly anymore, copying everything to a new HD and using it completely solve the errors.
I read that comment, but not sure what I have to do run tests against my external. Do you have more instructions written som
...
💬 MarcoFalke commented on pull request "bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee_rate":
(https://github.com/bitcoin/bitcoin/pull/27969#discussion_r1241602260)
missing test for larger than original fee rate, but not enough to be larger by incrementalrelayfee?
(https://github.com/bitcoin/bitcoin/pull/27969#discussion_r1241602260)
missing test for larger than original fee rate, but not enough to be larger by incrementalrelayfee?
💬 S3RK commented on pull request "Disable and uncheck blank when private keys are disabled":
(https://github.com/bitcoin-core/gui/pull/739#issuecomment-1606849299)
I'm nor a GUI code expert neither a UX (user experience) expert, so take all of this with a healthy skepticism.
The code changes look good to me, but I'm unsure about whether this is a better UX.
Code review ACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177
I'm unsure if it's a clear win for UX because there's no hint to the user why the checkboxes change their state. I also like the idea with radio buttons and I'd be happy to review a new PR or re-review this one though.
Maybe change the de
...
(https://github.com/bitcoin-core/gui/pull/739#issuecomment-1606849299)
I'm nor a GUI code expert neither a UX (user experience) expert, so take all of this with a healthy skepticism.
The code changes look good to me, but I'm unsure about whether this is a better UX.
Code review ACK 9ea31eba04ff8dcb9d7d993ce28bb10731a35177
I'm unsure if it's a clear win for UX because there's no hint to the user why the checkboxes change their state. I also like the idea with radio buttons and I'd be happy to review a new PR or re-review this one though.
Maybe change the de
...
📝 MarcoFalke opened a pull request: "util: Safer MakeByteSpan with ByteSpanCast "
(https://github.com/bitcoin/bitcoin/pull/27973)
The `AsBytes` helpers (or `std::as_bytes` helpers) are architecture
dependent. For example, the below test case diff [0], applied before
this commit will pass on x86_64, but fail on s390x [1].
Fix this by replacing `AsBytes` with a new `ByteSpanCast` in the `MakeByteSpan` helper.
This will turn the test case diff into a compile failure instead of a runtime error.
[0] test case diff:
```diff
diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
index 09f77d2b61..4
...
(https://github.com/bitcoin/bitcoin/pull/27973)
The `AsBytes` helpers (or `std::as_bytes` helpers) are architecture
dependent. For example, the below test case diff [0], applied before
this commit will pass on x86_64, but fail on s390x [1].
Fix this by replacing `AsBytes` with a new `ByteSpanCast` in the `MakeByteSpan` helper.
This will turn the test case diff into a compile failure instead of a runtime error.
[0] test case diff:
```diff
diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
index 09f77d2b61..4
...
🤔 MarcoFalke reviewed a pull request: "BIP324: ElligatorSwift integrations"
(https://github.com/bitcoin/bitcoin/pull/27479#pullrequestreview-1492430513)
left some style nits, feel free to ignore, unless you retouch for other reasons.
(https://github.com/bitcoin/bitcoin/pull/27479#pullrequestreview-1492430513)
left some style nits, feel free to ignore, unless you retouch for other reasons.
💬 MarcoFalke commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1238543437)
```suggestion
auto ret = key.EllSwiftCreate(MakeByteSpan(entropy));
```
style nit: Can use the alias which does the same, with the added bonus that the viewed data is const and future improvements to `MakeByteSpan` will benefit this call site as well.
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1238543437)
```suggestion
auto ret = key.EllSwiftCreate(MakeByteSpan(entropy));
```
style nit: Can use the alias which does the same, with the added bonus that the viewed data is const and future improvements to `MakeByteSpan` will benefit this call site as well.
💬 MarcoFalke commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1238043422)
style-nit: Obviously doesn't matter here, but could use `reinterpret_cast` to guard against accidentally removing constness. (And for symmetry with `UCharCast(const std::byte*)`)
```suggestion
inline unsigned char* UCharCast(std::byte* c) { return reinterpret_cast<unsigned char*>(c); }
```
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1238043422)
style-nit: Obviously doesn't matter here, but could use `reinterpret_cast` to guard against accidentally removing constness. (And for symmetry with `UCharCast(const std::byte*)`)
```suggestion
inline unsigned char* UCharCast(std::byte* c) { return reinterpret_cast<unsigned char*>(c); }
```