📝 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.
💬 MarcoFalke commented on pull request "MaybePunishNodeForTx: Remove unused message arg and logging":
(https://github.com/bitcoin/bitcoin/pull/27947#issuecomment-1607064993)
lgtm ACK 9fe5f6d5d1ec31ebfacfe23368f22c2a0b58832d 🕚
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 9fe5f6d5d1ec31eb
...
(https://github.com/bitcoin/bitcoin/pull/27947#issuecomment-1607064993)
lgtm ACK 9fe5f6d5d1ec31ebfacfe23368f22c2a0b58832d 🕚
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 9fe5f6d5d1ec31eb
...
💬 fanquake commented on pull request "Autogen 25.x":
(https://github.com/bitcoin/bitcoin/pull/27938#issuecomment-1607066605)
Thanks for the pull request, however it's unlikely we are going to make significant (refactoring) changes to our autotools based build system at this point.
(https://github.com/bitcoin/bitcoin/pull/27938#issuecomment-1607066605)
Thanks for the pull request, however it's unlikely we are going to make significant (refactoring) changes to our autotools based build system at this point.
✅ fanquake closed a pull request: "Autogen 25.x"
(https://github.com/bitcoin/bitcoin/pull/27938)
(https://github.com/bitcoin/bitcoin/pull/27938)
💬 tansanDOTeth commented on issue "Keep getting errors after a while of syncing":
(https://github.com/bitcoin/bitcoin/issues/27972#issuecomment-1607082182)
I tried reindexing since I posted this and I got this:
> 023-06-26T09:16:24Z UpdateTip: new best=00000000000000000c93572c7fcf0ccc927b5fb00039e90d1550327d6d83552f height=368600 version=0x00000003 log2_work=83.174292 tx=78966308 date='2015-08-06T04:29:51Z' progress=0.093234 cache=380.0MiB(2905123txo)
2023-06-26T09:16:25Z UpdateTip: new best=00000000000000000be13a49f9c0852b6c2c773efc816afadf4b2328bebc1b0f height=368601 version=0x00000003 log2_work=83.174322 tx=78967060 date='2015-08-06T04:36:12
...
(https://github.com/bitcoin/bitcoin/issues/27972#issuecomment-1607082182)
I tried reindexing since I posted this and I got this:
> 023-06-26T09:16:24Z UpdateTip: new best=00000000000000000c93572c7fcf0ccc927b5fb00039e90d1550327d6d83552f height=368600 version=0x00000003 log2_work=83.174292 tx=78966308 date='2015-08-06T04:29:51Z' progress=0.093234 cache=380.0MiB(2905123txo)
2023-06-26T09:16:25Z UpdateTip: new best=00000000000000000be13a49f9c0852b6c2c773efc816afadf4b2328bebc1b0f height=368601 version=0x00000003 log2_work=83.174322 tx=78967060 date='2015-08-06T04:36:12
...
💬 fanquake commented on issue "deadlock shutting down v25.0":
(https://github.com/bitcoin/bitcoin/issues/27965#issuecomment-1607090365)
@Crypt-iQ possibly similar to issues you've been seeing?
(https://github.com/bitcoin/bitcoin/issues/27965#issuecomment-1607090365)
@Crypt-iQ possibly similar to issues you've been seeing?
🚀 fanquake merged a pull request: "MaybePunishNodeForTx: Remove unused message arg and logging"
(https://github.com/bitcoin/bitcoin/pull/27947)
(https://github.com/bitcoin/bitcoin/pull/27947)
💬 MarcoFalke commented on issue "Keep getting errors after a while of syncing":
(https://github.com/bitcoin/bitcoin/issues/27972#issuecomment-1607094114)
Has anyone ever put the leveldb directory successfully on an external drive, I am not sure if this is even supported by leveldb?
(https://github.com/bitcoin/bitcoin/issues/27972#issuecomment-1607094114)
Has anyone ever put the leveldb directory successfully on an external drive, I am not sure if this is even supported by leveldb?
💬 MarcoFalke commented on issue "Keep getting errors after a while of syncing":
(https://github.com/bitcoin/bitcoin/issues/27972#issuecomment-1607099769)
In the meantime you can test your storage device use smartctl or CrystalDiskInfo to rule out any hardware defects.
(https://github.com/bitcoin/bitcoin/issues/27972#issuecomment-1607099769)
In the meantime you can test your storage device use smartctl or CrystalDiskInfo to rule out any hardware defects.
💬 tansanDOTeth commented on issue "Keep getting errors after a while of syncing":
(https://github.com/bitcoin/bitcoin/issues/27972#issuecomment-1607102513)
> In the meantime you can test your storage device use smartctl or CrystalDiskInfo to rule out any hardware defects.
I'll give that a try and report back
(https://github.com/bitcoin/bitcoin/issues/27972#issuecomment-1607102513)
> In the meantime you can test your storage device use smartctl or CrystalDiskInfo to rule out any hardware defects.
I'll give that a try and report back
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241919261)
My thought was that it might be good to choose a number of actually useable bytes that is a multiple of a commonly used data structure sizes such as 32 bytes. Therefore 256. It is an arbitrary number though that it supposed to strike a balance between the limiting the potency of annex inflation attacks and having enough space to make the annex useful.
Are you proposing to limit the annex size including any tags to 257 bytes? I see the conceptual difference, but there wouldn't be a functional
...
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241919261)
My thought was that it might be good to choose a number of actually useable bytes that is a multiple of a commonly used data structure sizes such as 32 bytes. Therefore 256. It is an arbitrary number though that it supposed to strike a balance between the limiting the potency of annex inflation attacks and having enough space to make the annex useful.
Are you proposing to limit the annex size including any tags to 257 bytes? I see the conceptual difference, but there wouldn't be a functional
...
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241927265)
With the constant defined as suggested below, this suggestion does seem to make reading easier. Changed.
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241927265)
With the constant defined as suggested below, this suggestion does seem to make reading easier. Changed.
👍 fanquake approved a pull request: "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM"
(https://github.com/bitcoin/bitcoin/pull/27676#pullrequestreview-1498126174)
ACK 3df60704661cdb5e61ea2b999f468f3a1d16105f
(https://github.com/bitcoin/bitcoin/pull/27676#pullrequestreview-1498126174)
ACK 3df60704661cdb5e61ea2b999f468f3a1d16105f
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241936380)
Added `MAX_PER_INPUT_ANNEX_SIZE`.
Not sure if a per tx maximum is necessary. I'd say that opting in to annex usage has some risks to be aware of. Maybe you don't want to use it for multi-party protocols at all because of that. To me, `MAX_PER_INPUT_ANNEX_SIZE` is already debatable when you have opt-in, and `MAX_ANNEX_BUDGET` feels even more like an unnecessary precaution. In the end, this is all policy.
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1241936380)
Added `MAX_PER_INPUT_ANNEX_SIZE`.
Not sure if a per tx maximum is necessary. I'd say that opting in to annex usage has some risks to be aware of. Maybe you don't want to use it for multi-party protocols at all because of that. To me, `MAX_PER_INPUT_ANNEX_SIZE` is already debatable when you have opt-in, and `MAX_ANNEX_BUDGET` feels even more like an unnecessary precaution. In the end, this is all policy.