✅ 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); }
```
💬 MarcoFalke commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1238546031)
```suggestion
auto ellswift = key.EllSwiftCreate(MakeByteSpan(ent32));
```
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1238546031)
```suggestion
auto ellswift = key.EllSwiftCreate(MakeByteSpan(ent32));
```
👍 dergoegge approved a pull request: "Remove the syscall sandbox"
(https://github.com/bitcoin/bitcoin/pull/27896#pullrequestreview-1497976492)
ACK 32e2ffc39374f61bb2435da507f285459985df9e
(https://github.com/bitcoin/bitcoin/pull/27896#pullrequestreview-1497976492)
ACK 32e2ffc39374f61bb2435da507f285459985df9e
✅ fanquake closed an issue: "rpc: signed-integer-overflow in analyzepsbt["estimated_feerate"]"
(https://github.com/bitcoin/bitcoin/issues/27913)
(https://github.com/bitcoin/bitcoin/issues/27913)
🚀 fanquake merged a pull request: "feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee"
(https://github.com/bitcoin/bitcoin/pull/27914)
(https://github.com/bitcoin/bitcoin/pull/27914)
📝 MarcoFalke converted_to_draft a pull request: "util: Allow std::byte and char Span serialization"
(https://github.com/bitcoin/bitcoin/pull/27927)
Seems odd to require developers to cast all byte-like spans passed to serialization to `unsigned char`-spans. Fix that by introducing a helper to convert any byte-like span to a `std::byte`-span, which is then accepted by serialization. Finally, add tests and update the code from `MakeUCharSpan`/`MakeByteSpan` to just `Span` where possible.
(https://github.com/bitcoin/bitcoin/pull/27927)
Seems odd to require developers to cast all byte-like spans passed to serialization to `unsigned char`-spans. Fix that by introducing a helper to convert any byte-like span to a `std::byte`-span, which is then accepted by serialization. Finally, add tests and update the code from `MakeUCharSpan`/`MakeByteSpan` to just `Span` where possible.
🚀 fanquake merged a pull request: "net: remove unused `CConnmanTest`"
(https://github.com/bitcoin/bitcoin/pull/27957)
(https://github.com/bitcoin/bitcoin/pull/27957)
⚠️ fanquake opened an issue: "ci: failure in `wallet_basic.py --descriptors`"
(https://github.com/bitcoin/bitcoin/issues/27974)
https://cirrus-ci.com/task/5029024158711808?logs=ci#L2900
```bash
node0 2023-06-23T16:51:00.034382Z [scheduler] [wallet/sqlite.cpp:47] [TraceSqlCallback] [/tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20230623_164625/wallet_basic_242/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?)
test 2023-06-23T16:51:00.043000Z TestFramework (ERROR): Assertion failed
Traceback (most recent cal
...
(https://github.com/bitcoin/bitcoin/issues/27974)
https://cirrus-ci.com/task/5029024158711808?logs=ci#L2900
```bash
node0 2023-06-23T16:51:00.034382Z [scheduler] [wallet/sqlite.cpp:47] [TraceSqlCallback] [/tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20230623_164625/wallet_basic_242/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?)
test 2023-06-23T16:51:00.043000Z TestFramework (ERROR): Assertion failed
Traceback (most recent cal
...
⚠️ dergoegge opened an issue: "fuzz: Use-of-uninitialized-value in evutil_inet_pton"
(https://github.com/bitcoin/bitcoin/issues/27975)
See https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56529.
We already have [workarounds](https://github.com/bitcoin/bitcoin/blob/80f04febbc6048f9da85b9289e0d433d1f164de0/src/test/fuzz/http_request.cpp#L37-L43) for similar issues in the `http_request` target, so this probably just needs another one of those.
(https://github.com/bitcoin/bitcoin/issues/27975)
See https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56529.
We already have [workarounds](https://github.com/bitcoin/bitcoin/blob/80f04febbc6048f9da85b9289e0d433d1f164de0/src/test/fuzz/http_request.cpp#L37-L43) for similar issues in the `http_request` target, so this probably just needs another one of those.
👍 dergoegge approved a pull request: "MaybePunishNodeForTx: Remove unused message arg and logging"
(https://github.com/bitcoin/bitcoin/pull/27947#pullrequestreview-1498042410)
utACK 9fe5f6d5d1ec31ebfacfe23368f22c2a0b58832d
(https://github.com/bitcoin/bitcoin/pull/27947#pullrequestreview-1498042410)
utACK 9fe5f6d5d1ec31ebfacfe23368f22c2a0b58832d
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1241879650)
> change this data struct to an array, add explicit numeric values to the Network enum (for index values)
That seems unnecessary, it is good to rely as less as possible on the actual values of the `Network` enums. That is - the code better not break if `NET_ONION` is not `3`, for example. Use them just as labels. `std::unordered_map` fits best with that - `std::unordered_map<Network, size_t>` would work even if `NET_*` are changed to be strings!
> ... protect the array with m_nodes_mutex t
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1241879650)
> change this data struct to an array, add explicit numeric values to the Network enum (for index values)
That seems unnecessary, it is good to rely as less as possible on the actual values of the `Network` enums. That is - the code better not break if `NET_ONION` is not `3`, for example. Use them just as labels. `std::unordered_map` fits best with that - `std::unordered_map<Network, size_t>` would work even if `NET_*` are changed to be strings!
> ... protect the array with m_nodes_mutex t
...
💬 MarcoFalke commented on pull request "util: Allow std::byte and char Span serialization":
(https://github.com/bitcoin/bitcoin/pull/27927#issuecomment-1607060383)
Rebased on https://github.com/bitcoin/bitcoin/pull/27973 for now, please review that first.
(https://github.com/bitcoin/bitcoin/pull/27927#issuecomment-1607060383)
Rebased on https://github.com/bitcoin/bitcoin/pull/27973 for now, please review that first.