💬 theuni commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934554861)
@dergoegge Ah, thanks for the correction. The option does indeed currently pass through to secp, so that bullet-point in my commit message is wrong and I'll update it.
It still forwards just fine with our option removed though, so I don't think that changes anything here.
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934554861)
@dergoegge Ah, thanks for the correction. The option does indeed currently pass through to secp, so that bullet-point in my commit message is wrong and I'll update it.
It still forwards just fine with our option removed though, so I don't think that changes anything here.
💬 araujo88 commented on issue "wallet_reorgrestore.py failed":
(https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1934559768)
@maflcko I'll continue our conversation here since https://github.com/bitcoin/bitcoin/pull/29395 was closed.
I'm not sure if I understood your idea clearly. We would add a sleep in the source code than run the related functional tests to try to force a race condition, or is the issue within the functional tests themselves? Are the source of the issue isn't clear yet for us to pin it down?
If you can point me in the right direction, I'll make some tweaking in the source code locally them po
...
(https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1934559768)
@maflcko I'll continue our conversation here since https://github.com/bitcoin/bitcoin/pull/29395 was closed.
I'm not sure if I understood your idea clearly. We would add a sleep in the source code than run the related functional tests to try to force a race condition, or is the issue within the functional tests themselves? Are the source of the issue isn't clear yet for us to pin it down?
If you can point me in the right direction, I'll make some tweaking in the source code locally them po
...
💬 theuni commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1934565305)
Concept ACK.
The linter output matches my list, with the additions of:
```
src/crypto/sha256_arm_shani.cpp
src/crypto/sha256_avx2.cpp
src/crypto/sha256_sse41.cpp
src/crypto/sha256_x86_shani.cpp
src/interfaces/wallet.h
```
The crypto defines actually come from our Makefile. Those have always been wonky, maybe now is a good time to do something about those.
`src/interfaces/wallet.h` however is a false-positive :(
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1934565305)
Concept ACK.
The linter output matches my list, with the additions of:
```
src/crypto/sha256_arm_shani.cpp
src/crypto/sha256_avx2.cpp
src/crypto/sha256_sse41.cpp
src/crypto/sha256_x86_shani.cpp
src/interfaces/wallet.h
```
The crypto defines actually come from our Makefile. Those have always been wonky, maybe now is a good time to do something about those.
`src/interfaces/wallet.h` however is a false-positive :(
💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483319279)
Sounds good
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483319279)
Sounds good
💬 ryanofsky commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934575281)
> Jup, that didn't work for me. Or maybe I did something wrong. See https://godbolt.org/z/z6EoWKbjY
It's not that surprising that the compiler adds padding so the struct member with uint64_t type is inside a field aligned to 8 bytes. But if you move the two bitfield members to begin on an 8 byte boundary (like between the nDataPos and nUndoPos fields, or to the beginning of the struct, it does get packed without extra padding). So I think the diff above is an easy way to avoid consuming extra
...
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934575281)
> Jup, that didn't work for me. Or maybe I did something wrong. See https://godbolt.org/z/z6EoWKbjY
It's not that surprising that the compiler adds padding so the struct member with uint64_t type is inside a field aligned to 8 bytes. But if you move the two bitfield members to begin on an 8 byte boundary (like between the nDataPos and nUndoPos fields, or to the beginning of the struct, it does get packed without extra padding). So I think the diff above is an easy way to avoid consuming extra
...
📝 dergoegge opened a pull request: "p2p: Don't process mutated blocks"
(https://github.com/bitcoin/bitcoin/pull/29412)
This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks.
We introduce `IsBlockMutated` which catches all known forms of block malleation and use it to do an early mutation check whenever we receive a `block` message.
We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. https://github.com/bitcoin/bitcoin/pull/27608 for which a regres
...
(https://github.com/bitcoin/bitcoin/pull/29412)
This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks.
We introduce `IsBlockMutated` which catches all known forms of block malleation and use it to do an early mutation check whenever we receive a `block` message.
We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. https://github.com/bitcoin/bitcoin/pull/27608 for which a regres
...
🤔 furszy reviewed a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1870828867)
Rebased after #26836 merge. Ready to go.
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1870828867)
Rebased after #26836 merge. Ready to go.
💬 maflcko commented on issue "wallet_reorgrestore.py failed":
(https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1934598134)
> Isn't the source of the issue clear
So far I haven't seen the source of the issue.
(https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1934598134)
> Isn't the source of the issue clear
So far I haven't seen the source of the issue.
💬 ajtowns commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934608651)
> > bitfields
>
> Jup, that didn't work for me. Or maybe I did something wrong. See https://godbolt.org/z/z6EoWKbjY
>
> ```
> 60:0-15 | int16_t nTx
> 64:0-47 | int64_t nChainTx
> ```
>
> (nTx eats byte 60-64, even though it is only a two byte bitfield)
Enclosing `nTx` and `nChainTx` in a sub-struct `struct hack { int16_t nTx : 16 {0}; int64_t nChainTx : 48 {0}; };` seems to fix this for me, fwiw.
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934608651)
> > bitfields
>
> Jup, that didn't work for me. Or maybe I did something wrong. See https://godbolt.org/z/z6EoWKbjY
>
> ```
> 60:0-15 | int16_t nTx
> 64:0-47 | int64_t nChainTx
> ```
>
> (nTx eats byte 60-64, even though it is only a two byte bitfield)
Enclosing `nTx` and `nChainTx` in a sub-struct `struct hack { int16_t nTx : 16 {0}; int64_t nChainTx : 48 {0}; };` seems to fix this for me, fwiw.
💬 ajtowns commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934619640)
> > bitfields
>
> Jup, that didn't work for me. Or maybe I did something wrong. See https://godbolt.org/z/z6EoWKbjY
>
> ```
> 60:0-15 | int16_t nTx
> 64:0-47 | int64_t nChainTx
> ```
>
> (nTx eats byte 60-64, even though it is only a two byte bitfield)
I think the problem here is you have an odd number of 32-bit pointers prior to `nTx`, so it's misaligned to be combined with `nChainTx`. So you end up with `nTx [4B-6B padding] nChainTx nStatus [4B padding]`. Moving `nStat
...
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934619640)
> > bitfields
>
> Jup, that didn't work for me. Or maybe I did something wrong. See https://godbolt.org/z/z6EoWKbjY
>
> ```
> 60:0-15 | int16_t nTx
> 64:0-47 | int64_t nChainTx
> ```
>
> (nTx eats byte 60-64, even though it is only a two byte bitfield)
I think the problem here is you have an odd number of 32-bit pointers prior to `nTx`, so it's misaligned to be combined with `nChainTx`. So you end up with `nTx [4B-6B padding] nChainTx nStatus [4B padding]`. Moving `nStat
...
💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1934662433)
> `src/interfaces/wallet.h` however is a false-positive :(
Yeah, not sure how to solve this. Maybe rewrite everything as a clang-tidy plugin?
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1934662433)
> `src/interfaces/wallet.h` however is a false-positive :(
Yeah, not sure how to solve this. Maybe rewrite everything as a clang-tidy plugin?
💬 maflcko commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-1934672607)
https://cirrus-ci.com/task/6533368603475968?logs=ci#L3401
```
node0 stderr bitcoin-node: wallet/wallet.cpp:246: void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion `it.second' failed.
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-1934672607)
https://cirrus-ci.com/task/6533368603475968?logs=ci#L3401
```
node0 stderr bitcoin-node: wallet/wallet.cpp:246: void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion `it.second' failed.
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1934674471)
I added tests for `RequestTransactionData` to `2024/02/sv2-poll-ellswift`, including an RBF test. It seems that the node _does_ hold on to transaction data after an RBF. I don't know if that's because the block template has a p pointer, or if there's some other mechanism at play.
We'll need some way to limit the amount of memory that this can take up.
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1934674471)
I added tests for `RequestTransactionData` to `2024/02/sv2-poll-ellswift`, including an RBF test. It seems that the node _does_ hold on to transaction data after an RBF. I don't know if that's because the block template has a p pointer, or if there's some other mechanism at play.
We'll need some way to limit the amount of memory that this can take up.
✅ maflcko closed an issue: "When I make a highly concurrent request to bitcoin core, a new block appears and all my requests get blocked"
(https://github.com/bitcoin/bitcoin/issues/29384)
(https://github.com/bitcoin/bitcoin/issues/29384)
💬 maflcko commented on issue "When I make a highly concurrent request to bitcoin core, a new block appears and all my requests get blocked":
(https://github.com/bitcoin/bitcoin/issues/29384#issuecomment-1934675409)
Closing for now. Let us know if this is still an issue after https://github.com/bitcoin/bitcoin/issues/29384#issuecomment-1929130106
(https://github.com/bitcoin/bitcoin/issues/29384#issuecomment-1934675409)
Closing for now. Let us know if this is still an issue after https://github.com/bitcoin/bitcoin/issues/29384#issuecomment-1929130106
💬 achow101 commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1934678359)
ACK fab41697a5448ef2861f65795bd63a4ccdda6a40
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1934678359)
ACK fab41697a5448ef2861f65795bd63a4ccdda6a40
💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483394708)
Sorry, my bad, looking at the diff I misinterpreted the definition of initialize that is right bellow `NetPermissions::ClearFlag` as a call to it. Disregard this commet.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483394708)
Sorry, my bad, looking at the diff I misinterpreted the definition of initialize that is right bellow `NetPermissions::ClearFlag` as a call to it. Disregard this commet.
💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483395218)
Sorry, my bad, looking at the diff I misinterpreted the definition of initialize that is right bellow NetPermissions::ClearFlag as a call to it. Disregard this commet.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483395218)
Sorry, my bad, looking at the diff I misinterpreted the definition of initialize that is right bellow NetPermissions::ClearFlag as a call to it. Disregard this commet.
💬 maflcko commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1934682164)
Bitcoin Core makes heavy use of CPU, RAM and disk IO. Hardware defects might only become visible when running Bitcoin Core. You might want to check your hardware for defects.
* memtest86 to check your RAM
* to check the CPU behaviour under load, use linpack or Prime95
* to test your storage device use smartctl or CrystalDiskInfo
Source: https://bitcoin.stackexchange.com/a/12206
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1934682164)
Bitcoin Core makes heavy use of CPU, RAM and disk IO. Hardware defects might only become visible when running Bitcoin Core. You might want to check your hardware for defects.
* memtest86 to check your RAM
* to check the CPU behaviour under load, use linpack or Prime95
* to test your storage device use smartctl or CrystalDiskInfo
Source: https://bitcoin.stackexchange.com/a/12206
💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483399472)
I guess that depends on what feedback we want to provide the user when an invalid configuration has been set (i.e. silently dropping vs being loud about it).
This is quite similar to other misconfigurations, such as setting the same permission twice, but with the particularity that it actually cannot be applied by itself, so maybe it's worth returning an error in this case
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483399472)
I guess that depends on what feedback we want to provide the user when an invalid configuration has been set (i.e. silently dropping vs being loud about it).
This is quite similar to other misconfigurations, such as setting the same permission twice, but with the particularity that it actually cannot be applied by itself, so maybe it's worth returning an error in this case