💬 maflcko commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934519682)
> 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)
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934519682)
> 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)
💬 dergoegge commented on issue "p2p: lingering entries in `mapBlockSource`":
(https://github.com/bitcoin/bitcoin/issues/29410#issuecomment-1934529375)
I think removing entries from `mapBlockSource` once the corresponding peers disconnect would make the most sense because we want to be able to punish the source in case the block turns out to be invalid. If we expire or otherwise prematurely remove from the map we loose that ability.
Something like the following in `FinalizeNode`:
```c++
for (auto it = mapBlockSource.begin(); it != mapBlockSource.end();) {
auto& [hash, entry] = *it;
if (entry.first == node.GetId()) {
...
(https://github.com/bitcoin/bitcoin/issues/29410#issuecomment-1934529375)
I think removing entries from `mapBlockSource` once the corresponding peers disconnect would make the most sense because we want to be able to punish the source in case the block turns out to be invalid. If we expire or otherwise prematurely remove from the map we loose that ability.
Something like the following in `FinalizeNode`:
```c++
for (auto it = mapBlockSource.begin(); it != mapBlockSource.end();) {
auto& [hash, entry] = *it;
if (entry.first == node.GetId()) {
...
🤔 ryanofsky reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1870720275)
Updated 2f4e7e8dc82adb60a339211cdc15408b5a6919ce -> c9ca4ca4d970f9d42d0254775c04d114bd2152c3 ([`pr/nofake.6`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.6) -> [`pr/nofake.7`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nofake.6..pr/nofake.7)) added suggested unset nChainTx check in GuessVerificationProgress
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1870720275)
Updated 2f4e7e8dc82adb60a339211cdc15408b5a6919ce -> c9ca4ca4d970f9d42d0254775c04d114bd2152c3 ([`pr/nofake.6`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.6) -> [`pr/nofake.7`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nofake.6..pr/nofake.7)) added suggested unset nChainTx check in GuessVerificationProgress
💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1483275071)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479169504
> unrelated to this pull: Maybe an `Assume(nchaintx)` can be added to `GuessVerificationProgress`?
I went ahead and added this in a separate commit
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1483275071)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479169504
> unrelated to this pull: Maybe an `Assume(nchaintx)` can be added to `GuessVerificationProgress`?
I went ahead and added this in a separate commit
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483298313)
I don't think so, otherwise this is needlessly making things worse than it needs to be.
Imagine transactions C1 and C2 are both v3, both in the mempool, with confirmed v3 parent P. C2 can be arbitrarily big, since it entered the mempool with no unconfirmed parents.
P is reorged out, and now P, C1, and C2 are all in the mempool.
Suppose D is an RBF of C1 that would pass our RBF rules. However, C2 is a big transaction, and being forced to conflict with C2 (eg due to sibling eviction or
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483298313)
I don't think so, otherwise this is needlessly making things worse than it needs to be.
Imagine transactions C1 and C2 are both v3, both in the mempool, with confirmed v3 parent P. C2 can be arbitrarily big, since it entered the mempool with no unconfirmed parents.
P is reorged out, and now P, C1, and C2 are all in the mempool.
Suppose D is an RBF of C1 that would pass our RBF rules. However, C2 is a big transaction, and being forced to conflict with C2 (eg due to sibling eviction or
...
💬 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