💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453643762)
Will leave as is then. Seems likely that something will end up here.
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453643762)
Will leave as is then. Seems likely that something will end up here.
👍 fanquake approved a pull request: "refactor: C++20: Use std::rotl"
(https://github.com/bitcoin/bitcoin/pull/29085#pullrequestreview-1824029448)
ACK 60446285436da62adef1c0a9b11c3336d82b4d89
Had a look at the difference in master vs this PR (rebased) on aarch64. Seems like for the most part, this gives the same output +- a handful of instructions. (Identical is identical minus difference in addresses / argument ordering etc).
| Function | GCC 13.2 | Clang 17.0.6 |
| --- | --- | --- |
| MurmurHash3 | Identical | PR gives 5 less instructions: https://www.diffchecker.com/Jmlpl4rG/ |
| ChaCha20Aligned::Keystream | PR gives 1 less in
...
(https://github.com/bitcoin/bitcoin/pull/29085#pullrequestreview-1824029448)
ACK 60446285436da62adef1c0a9b11c3336d82b4d89
Had a look at the difference in master vs this PR (rebased) on aarch64. Seems like for the most part, this gives the same output +- a handful of instructions. (Identical is identical minus difference in addresses / argument ordering etc).
| Function | GCC 13.2 | Clang 17.0.6 |
| --- | --- | --- |
| MurmurHash3 | Identical | PR gives 5 less instructions: https://www.diffchecker.com/Jmlpl4rG/ |
| ChaCha20Aligned::Keystream | PR gives 1 less in
...
💬 sipa commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1894084086)
@fanquake Which side of the diff is which version?
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1894084086)
@fanquake Which side of the diff is which version?
💬 fanquake commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1894088819)
> Which side of the diff is which version?
Left is master @ f1fcc9638cde7664b9642018fe6872148bbb0172 , right was the PR based on f1fcc9638cde7664b9642018fe6872148bbb0172.
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1894088819)
> Which side of the diff is which version?
Left is master @ f1fcc9638cde7664b9642018fe6872148bbb0172 , right was the PR based on f1fcc9638cde7664b9642018fe6872148bbb0172.
💬 murchandamus commented on pull request "test: Add algo assert to bnb_search_test":
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1894112305)
In the test case the `9` is pre-selected and therefore has to be used. This is why BnB returns 9+1 instead of 10.
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1894112305)
In the test case the `9` is pre-selected and therefore has to be used. This is why BnB returns 9+1 instead of 10.
💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453696973)
clang-tidy doesn't like it being left there. Its suggestion of defaulting it in the header seems counter to what we want to achieve here.
```
/home/drgrid/bitcoin/src/./kernel/context.h:22:5: error: class 'Context' can be made trivially destructible by defaulting the destructor on its first declaration [performance-trivially-destructible,-warnings-as-errors]
~Context();
^
= default
kernel/context.cpp:23:10: note: destructor definition is here
Context::~Context() = d
...
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453696973)
clang-tidy doesn't like it being left there. Its suggestion of defaulting it in the header seems counter to what we want to achieve here.
```
/home/drgrid/bitcoin/src/./kernel/context.h:22:5: error: class 'Context' can be made trivially destructible by defaulting the destructor on its first declaration [performance-trivially-destructible,-warnings-as-errors]
~Context();
^
= default
kernel/context.cpp:23:10: note: destructor definition is here
Context::~Context() = d
...
🤔 murchandamus reviewed a pull request: "test: Add algo assert to bnb_search_test"
(https://github.com/bitcoin/bitcoin/pull/29206#pullrequestreview-1824147084)
Concept NACK
Tests should ensure that the expected outcome is achieved. The expected outcome here is that the coin selection process returns the input set that has the best waste score.
I don’t think it’s useful to require that the best input set was produced by BnB. While in our current implementation the best input set is guaranteed to be found by BnB, it is possible that future coin selection improvements would enable another algorithm to produce the same result. As such, requiring tha
...
(https://github.com/bitcoin/bitcoin/pull/29206#pullrequestreview-1824147084)
Concept NACK
Tests should ensure that the expected outcome is achieved. The expected outcome here is that the coin selection process returns the input set that has the best waste score.
I don’t think it’s useful to require that the best input set was produced by BnB. While in our current implementation the best input set is guaranteed to be found by BnB, it is possible that future coin selection improvements would enable another algorithm to produce the same result. As such, requiring tha
...
💬 theuni commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894141296)
FWIW @hebasto's solution looks correct (and necessary) to me.
An alternative would be to use c++20's `std::assume_aligned` if we could guarantee the input's alignment, but looking at the usage that's definitely not the case.
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894141296)
FWIW @hebasto's solution looks correct (and necessary) to me.
An alternative would be to use c++20's `std::assume_aligned` if we could guarantee the input's alignment, but looking at the usage that's definitely not the case.
💬 jamesob commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1894142760)
Approach ACK; seems like a reasonable feature reasonably implemented. Will do a more formal review soon.
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1894142760)
Approach ACK; seems like a reasonable feature reasonably implemented. Will do a more formal review soon.
💬 ns-xvrn commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1894154090)
I strongly think that datacarriersize should be updated what it truly supposed to do in the first place(not what it's documentation was changed to after ord spam started). This is like saying that every option that exists now literally doesn't apply to future additions in the functionality.
And people who say it's a cat and mouse game, isn't every attack mitigation in network security like that?
May be you want to admit that there isn't a good solution you've found but even then what makes yo
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1894154090)
I strongly think that datacarriersize should be updated what it truly supposed to do in the first place(not what it's documentation was changed to after ord spam started). This is like saying that every option that exists now literally doesn't apply to future additions in the functionality.
And people who say it's a cat and mouse game, isn't every attack mitigation in network security like that?
May be you want to admit that there isn't a good solution you've found but even then what makes yo
...
💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1894164176)
ACK 79e10351b1ce1f8db98ece67aacc24f323008b37
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1894164176)
ACK 79e10351b1ce1f8db98ece67aacc24f323008b37
💬 fanquake commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894168783)
@hebasto Can you update https://github.com/bitcoin-core/crc32c-subtree/pull/6 so that the commit includes a description of the problem, an explanation of the fix, and any further information. We should probably add a comment inline explaining why a memcpy is being added to that code.
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894168783)
@hebasto Can you update https://github.com/bitcoin-core/crc32c-subtree/pull/6 so that the commit includes a description of the problem, an explanation of the fix, and any further information. We should probably add a comment inline explaining why a memcpy is being added to that code.
💬 theuni commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894185292)
It'd be worth taking upstream as well.
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894185292)
It'd be worth taking upstream as well.
💬 fanquake commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894191530)
> It'd be worth taking upstream as well.
We could open a PR, but I'm not sure if we'll get any traction there. The last change upstream https://github.com/google/crc32c was ~ 3 years ago, and the readme recommends the use of https://github.com/google/highwayhash, which is actively maintained.
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894191530)
> It'd be worth taking upstream as well.
We could open a PR, but I'm not sure if we'll get any traction there. The last change upstream https://github.com/google/crc32c was ~ 3 years ago, and the readme recommends the use of https://github.com/google/highwayhash, which is actively maintained.
💬 achow101 commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1894209088)
ACK df30247705940c50c5eaafd74e2abbeb8b0cec07
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1894209088)
ACK df30247705940c50c5eaafd74e2abbeb8b0cec07
🚀 achow101 merged a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179)
(https://github.com/bitcoin/bitcoin/pull/29179)
💬 achow101 commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1894299113)
ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1894299113)
ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
💬 yancyribbens commented on pull request "test: Add algo assert to bnb_search_test":
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1894307538)
> I don’t think it’s useful to require that the best input set was produced by BnB
That's fine, however the tests in question are part of the `bnb_search_test`. So if it's not testing BnB, I feel like the tests should be moved to a different test. It's confusing imo to understand how BnB works if the tests cases labeled as `bnb_search_test` are not acutally testing `bnb_search`.
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1894307538)
> I don’t think it’s useful to require that the best input set was produced by BnB
That's fine, however the tests in question are part of the `bnb_search_test`. So if it's not testing BnB, I feel like the tests should be moved to a different test. It's confusing imo to understand how BnB works if the tests cases labeled as `bnb_search_test` are not acutally testing `bnb_search`.
🚀 achow101 merged a pull request: "doc, test: test and explain service flag handling"
(https://github.com/bitcoin/bitcoin/pull/29213)
(https://github.com/bitcoin/bitcoin/pull/29213)
⚠️ Xaspr opened an issue: "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error"
(https://github.com/bitcoin/bitcoin/issues/29255)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
After installing Bitcoin Core 26.0 in Windows Pro 11 and starting IBD, Core crashes.
I know Windows might not be exactly ideal, but I like to run a pruned node on my work laptop as well. I didn't set pruning yet by the way, I started Core just with the standard settings.
Bitcoin Core crashes and the following error pops up in debug.log:
`ERROR: ReadBlockFromDisk: Deserialize or
...
(https://github.com/bitcoin/bitcoin/issues/29255)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
After installing Bitcoin Core 26.0 in Windows Pro 11 and starting IBD, Core crashes.
I know Windows might not be exactly ideal, but I like to run a pruned node on my work laptop as well. I didn't set pruning yet by the way, I started Core just with the standard settings.
Bitcoin Core crashes and the following error pops up in debug.log:
`ERROR: ReadBlockFromDisk: Deserialize or
...