💬 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
...
💬 maflcko commented on pull request "test: Add and use option for tx-version in MiniWallet methods":
(https://github.com/bitcoin/bitcoin/pull/28972#issuecomment-1894333952)
rebased for CI
(https://github.com/bitcoin/bitcoin/pull/28972#issuecomment-1894333952)
rebased for CI
💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1894340133)
Post-merge ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1894340133)
Post-merge ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453887711)
It is possible to suppress a clang tidy check on a specific line of code
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453887711)
It is possible to suppress a clang tidy check on a specific line of code
💬 murchandamus commented on pull request "test: Add algo assert to bnb_search_test":
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1894388979)
The presence of BnB ensures that these expected input set will be found. The presence of BnB does neither preclude another algorithm from finding the same solution, nor cause the BnB solution to be necessarily picked among two equivalent solutions produced by two different algorithms. Hence it does not make sense to require that the solution was produced by BnB, only that the best solution will be found.
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1894388979)
The presence of BnB ensures that these expected input set will be found. The presence of BnB does neither preclude another algorithm from finding the same solution, nor cause the BnB solution to be necessarily picked among two equivalent solutions produced by two different algorithms. Hence it does not make sense to require that the solution was produced by BnB, only that the best solution will be found.
💬 hebasto commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1894391840)
Does it happen with the previous versions: 25.1, 24.2?
Does it happen, when antivirus tools/software are disabled (or the datadir is added to the excluded folders)?
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1894391840)
Does it happen with the previous versions: 25.1, 24.2?
Does it happen, when antivirus tools/software are disabled (or the datadir is added to the excluded folders)?
📝 murchandamus converted_to_draft a pull request: "Fix waste calculation in SelectionResult"
(https://github.com/bitcoin/bitcoin/pull/28366)
PR #26152 moved waste calculation into SelectionResult to be able to correct the waste score on basis of the bump_fee_group_discount for overlapping ancestries. This left two functions with largely overlapping purpose, where one was simply a wrapper of the other. This PR cleans up the overlap, and fixes the double-meaning of `change_cost` where the `GetChange()` function assumed that no change was created when `change_cost` was set to 0. This behavior was exploited in a bunch of tests, but is pr
...
(https://github.com/bitcoin/bitcoin/pull/28366)
PR #26152 moved waste calculation into SelectionResult to be able to correct the waste score on basis of the bump_fee_group_discount for overlapping ancestries. This left two functions with largely overlapping purpose, where one was simply a wrapper of the other. This PR cleans up the overlap, and fixes the double-meaning of `change_cost` where the `GetChange()` function assumed that no change was created when `change_cost` was set to 0. This behavior was exploited in a bunch of tests, but is pr
...
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#issuecomment-1894414662)
Converting to draft until I get around to fixing the coin_selection benchmark.
(https://github.com/bitcoin/bitcoin/pull/28366#issuecomment-1894414662)
Converting to draft until I get around to fixing the coin_selection benchmark.