💬 theuni commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2919662378)
> https://cirrus-ci.com/task/6734127736553472?logs=ci#L3223
>
> ```shell
> [12:12:21.305] /ci_container_base/src/test/reverselock_tests.cpp:23:9: error: cannot call function 'AssertLockNotHeldInline' while mutex 'mutex' is held [-Werror,-Wthread-safety-analysis]
> [12:12:21.305] 23 | AssertLockNotHeld(mutex);
> [12:12:21.305] | ^
> [12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
> [12:12:21.305] 141 | #define A
...
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2919662378)
> https://cirrus-ci.com/task/6734127736553472?logs=ci#L3223
>
> ```shell
> [12:12:21.305] /ci_container_base/src/test/reverselock_tests.cpp:23:9: error: cannot call function 'AssertLockNotHeldInline' while mutex 'mutex' is held [-Werror,-Wthread-safety-analysis]
> [12:12:21.305] 23 | AssertLockNotHeld(mutex);
> [12:12:21.305] | ^
> [12:12:21.305] /ci_container_base/src/sync.h:141:31: note: expanded from macro 'AssertLockNotHeld'
> [12:12:21.305] 141 | #define A
...
⚠️ rando876 opened an issue: "Bug in PartiallyDownloadedBlock fuzz test logic or behavior"
(https://github.com/bitcoin/bitcoin/issues/32640)
There may be a bug in the fuzz test implementation for `PartiallyDownloadedBlock` in `src/test/fuzz/partially_downloaded_block.cpp`. Please investigate the following:
- The fuzz test performs various operations relating to compact blocks and partial downloads, including marking transactions as available, manipulating mempool/extra_txn, and reconstructing blocks.
- It uses assertions to check the state after calling `InitData` and `FillBlock` on `PartiallyDownloadedBlock`.
- The test may not pro
...
(https://github.com/bitcoin/bitcoin/issues/32640)
There may be a bug in the fuzz test implementation for `PartiallyDownloadedBlock` in `src/test/fuzz/partially_downloaded_block.cpp`. Please investigate the following:
- The fuzz test performs various operations relating to compact blocks and partial downloads, including marking transactions as available, manipulating mempool/extra_txn, and reconstructing blocks.
- It uses assertions to check the state after calling `InitData` and `FillBlock` on `PartiallyDownloadedBlock`.
- The test may not pro
...
🤔 instagibbs reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2878472725)
reviewed through ea3a65e698f519afee23484ce1b399e9a4c62529
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2878472725)
reviewed through ea3a65e698f519afee23484ce1b399e9a4c62529
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114070740)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
```Suggestion
node::TxOrphanageImpl orphanage_low_mem(/*max_global_ann=*/10, /*reserved_peer_usage=*/TX_SIZE);
```
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114070740)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
```Suggestion
node::TxOrphanageImpl orphanage_low_mem(/*max_global_ann=*/10, /*reserved_peer_usage=*/TX_SIZE);
```
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114084831)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
might as well use and assert return of `CheckNumEvictions`
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114084831)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
might as well use and assert return of `CheckNumEvictions`
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114089251)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
nit: s/peer0/dos_peer/
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114089251)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
nit: s/peer0/dos_peer/
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114080543)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
could run these checks just before additions to assert no-op
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114080543)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
could run these checks just before additions to assert no-op
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114103109)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
`EraseForPeer` and `EraseTx` coverage here would be good. IIUC max memory goes up IFF a peer has a live orphan announcement?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114103109)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
`EraseForPeer` and `EraseTx` coverage here would be good. IIUC max memory goes up IFF a peer has a live orphan announcement?
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114084722)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
might as well use and assert return of `CheckNumEvictions`
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114084722)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
might as well use and assert return of `CheckNumEvictions`
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114085090)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
might as well use and assert return of `CheckNumEvictions`
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114085090)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
might as well use and assert return of `CheckNumEvictions`
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114091408)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
```Suggestion
BOOST_CHECK_EQUAL(orphanage.AnnouncementsFromPeer(peer1), 0);
BOOST_CHECK_EQUAL(orphanage.AnnouncementsFromPeer(peer2), 0);
```
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114091408)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
```Suggestion
BOOST_CHECK_EQUAL(orphanage.AnnouncementsFromPeer(peer1), 0);
BOOST_CHECK_EQUAL(orphanage.AnnouncementsFromPeer(peer2), 0);
```
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114104811)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
Single scenario that boots out 2+ txns with a single `CheckNumEvictions` seems apt
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114104811)
c9a34d4cdb5f5eca385304dc5c836960fad2a74a
Single scenario that boots out 2+ txns with a single `CheckNumEvictions` seems apt
💬 fanquake commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2919737881)
Actually, we can now use 0.16.6: https://github.com/lief-project/LIEF/releases/tag/0.16.6, which includes that change.
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2919737881)
Actually, we can now use 0.16.6: https://github.com/lief-project/LIEF/releases/tag/0.16.6, which includes that change.
💬 ryanofsky commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2919753784)
> The constness of methods is a property of the interface. It is a way to say calling it will not change the object
Yes, I understand the intent of marking `interfaces::BlockTemplate` methods `const` is to say calling those methods will not change the object. And I agree using `const` to prevent changing the object would be a nice goal, but unfortunately that's not what `const` keyword does here.
Marking the methods `const` does not mark the block template data `const` or do anything that
...
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2919753784)
> The constness of methods is a property of the interface. It is a way to say calling it will not change the object
Yes, I understand the intent of marking `interfaces::BlockTemplate` methods `const` is to say calling those methods will not change the object. And I agree using `const` to prevent changing the object would be a nice goal, but unfortunately that's not what `const` keyword does here.
Marking the methods `const` does not mark the block template data `const` or do anything that
...
📝 fanquake converted_to_draft a pull request: "Mining: Avoid copying template CBlocks"
(https://github.com/bitcoin/bitcoin/pull/32547)
Refactoring to avoid unnecessary copies/complexity, at least in the mainnet paths.
Also abstracts out a new `TemplateToJSON` function to keep track of variable scoping better.
(https://github.com/bitcoin/bitcoin/pull/32547)
Refactoring to avoid unnecessary copies/complexity, at least in the mainnet paths.
Also abstracts out a new `TemplateToJSON` function to keep track of variable scoping better.
💬 fanquake commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2919777106)
Backported to `27.x` in #32479.
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2919777106)
Backported to `27.x` in #32479.
💬 fanquake commented on pull request "[28.x] Backport guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32639#issuecomment-2919815304)
Guix Build:
```bash
f68d83b36bec27fd42c754af357d205c9abf7e2e90b69e7d1f9136813a191745 guix-build-5c2ba9f583e2/output/aarch64-linux-gnu/SHA256SUMS.part
e29852e1e6905d2c90734c39cf2994680b83228148342e25b0f44b44a6962a1a guix-build-5c2ba9f583e2/output/aarch64-linux-gnu/bitcoin-5c2ba9f583e2-aarch64-linux-gnu-debug.tar.gz
949df5bf91e782332065362d02b77ea8f2686433eb461e2458b91996cf97cd8f guix-build-5c2ba9f583e2/output/aarch64-linux-gnu/bitcoin-5c2ba9f583e2-aarch64-linux-gnu.tar.gz
eb18222f14ad9cec
...
(https://github.com/bitcoin/bitcoin/pull/32639#issuecomment-2919815304)
Guix Build:
```bash
f68d83b36bec27fd42c754af357d205c9abf7e2e90b69e7d1f9136813a191745 guix-build-5c2ba9f583e2/output/aarch64-linux-gnu/SHA256SUMS.part
e29852e1e6905d2c90734c39cf2994680b83228148342e25b0f44b44a6962a1a guix-build-5c2ba9f583e2/output/aarch64-linux-gnu/bitcoin-5c2ba9f583e2-aarch64-linux-gnu-debug.tar.gz
949df5bf91e782332065362d02b77ea8f2686433eb461e2458b91996cf97cd8f guix-build-5c2ba9f583e2/output/aarch64-linux-gnu/bitcoin-5c2ba9f583e2-aarch64-linux-gnu.tar.gz
eb18222f14ad9cec
...
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114242408)
what if the peer sent us the first non-reconsidered orphan? would that not be considered below due to sequence number being 0?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2114242408)
what if the peer sent us the first non-reconsidered orphan? would that not be considered below due to sequence number being 0?
💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2114255993)
I have benchmarked both approaches (reading block undo data directly from storage and returning re-serialized spent txouts) using https://github.com/bitcoin/bitcoin/pull/32540/commits/d5828eadc39772bf825eb4141340a281f4a490a7 with https://github.com/romanz/bench-rest/commit/f90aaddf1c3a0c0687f373cc89b0c343e35df739:
## Fetching block undo data (using `ReadRawBlockUndo`)
```
$ time cargo run --release --bin blockundo
Finished `release` profile [optimized] target(s) in 0.02s
Running
...
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2114255993)
I have benchmarked both approaches (reading block undo data directly from storage and returning re-serialized spent txouts) using https://github.com/bitcoin/bitcoin/pull/32540/commits/d5828eadc39772bf825eb4141340a281f4a490a7 with https://github.com/romanz/bench-rest/commit/f90aaddf1c3a0c0687f373cc89b0c343e35df739:
## Fetching block undo data (using `ReadRawBlockUndo`)
```
$ time cargo run --release --bin blockundo
Finished `release` profile [optimized] target(s) in 0.02s
Running
...
✅ fanquake closed an issue: "Bug in PartiallyDownloadedBlock fuzz test logic or behavior"
(https://github.com/bitcoin/bitcoin/issues/32640)
(https://github.com/bitcoin/bitcoin/issues/32640)