🤔 pinheadmz requested changes to a pull request: "Update best_header inside Connect/DisconnectTip"
(https://github.com/bitcoin/bitcoin/pull/26260#pullrequestreview-1423237913)
concept ACK
one question below. I haven't checked yet but I wonder if https://github.com/bitcoin/bitcoin/issues/10439 is a related issue, or how hard it would be to knock out that bug while we're working here already.
(https://github.com/bitcoin/bitcoin/pull/26260#pullrequestreview-1423237913)
concept ACK
one question below. I haven't checked yet but I wonder if https://github.com/bitcoin/bitcoin/issues/10439 is a related issue, or how hard it would be to knock out that bug while we're working here already.
💬 pinheadmz commented on pull request "Update best_header inside Connect/DisconnectTip":
(https://github.com/bitcoin/bitcoin/pull/26260#discussion_r1191571917)
Tests still pass with this chunk removed. I wonder if checking that getblockchaininfo `headers == blocks` isn't enough? Might need to use `getchaintips` to cover.
(https://github.com/bitcoin/bitcoin/pull/26260#discussion_r1191571917)
Tests still pass with this chunk removed. I wonder if checking that getblockchaininfo `headers == blocks` isn't enough? Might need to use `getchaintips` to cover.
💬 pinheadmz commented on pull request "Update best_header inside Connect/DisconnectTip":
(https://github.com/bitcoin/bitcoin/pull/26260#discussion_r1191575112)
micro-nit, this is a silly variable name. i think it's fine, but it is silly.
(https://github.com/bitcoin/bitcoin/pull/26260#discussion_r1191575112)
micro-nit, this is a silly variable name. i think it's fine, but it is silly.
💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191578999)
s/FLUSH_INTERVAL/FEE_FLUSH_INTERVAL/ ?
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191578999)
s/FLUSH_INTERVAL/FEE_FLUSH_INTERVAL/ ?
💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191584552)
FEE_FLUSH_INTERVAL is better.
I think.
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191584552)
FEE_FLUSH_INTERVAL is better.
I think.
💬 MarcoFalke commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544546730)
> we could trim mapRelay before their scheduled expiry time has actually been reached.
Sounds like this would either degrade compact block relay when the mempool is full due to trimming `mapRelay`, or turn one DoS (OOM) into another DoS (lost fee income) due to trimming valid mempool txs? And if you use separate and dedicated size limits for `mapRelay` and the mempool, it might be easier to keep them separate data structures?
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544546730)
> we could trim mapRelay before their scheduled expiry time has actually been reached.
Sounds like this would either degrade compact block relay when the mempool is full due to trimming `mapRelay`, or turn one DoS (OOM) into another DoS (lost fee income) due to trimming valid mempool txs? And if you use separate and dedicated size limits for `mapRelay` and the mempool, it might be easier to keep them separate data structures?
💬 glozow commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544548047)
> > > `txinfo.m_time <= mempool_req`
>
> > This condition is checking that our inv (`txinfo`) is more recent than the last `mempool_req`, meaning that once a mempool request has been processed the peer can request any transaction from our mempool.
>
> I don't get it. Is it not that the condition is checking whether `txinfo` is _older_ than the last `mempool_req`? In that case the transaction has been part of the `MEMPOOL` reply, so we have sent an `INV` about it.
Ah, I think you're righ
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544548047)
> > > `txinfo.m_time <= mempool_req`
>
> > This condition is checking that our inv (`txinfo`) is more recent than the last `mempool_req`, meaning that once a mempool request has been processed the peer can request any transaction from our mempool.
>
> I don't get it. Is it not that the condition is checking whether `txinfo` is _older_ than the last `mempool_req`? In that case the transaction has been part of the `MEMPOOL` reply, so we have sent an `INV` about it.
Ah, I think you're righ
...
💬 glozow commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191600264)
What I mean is
Leak available without mapRelay:
1. spy sends txA. It's added to mempool.
2. spy sends txB (through the network) and continuously queries our node for txA
3. once we accept txB to mempool, we serve notfound for txA (because it's not there anymore). spy learns exactly what time we received txB.
^This is prevented with mapRelay because after txA leaves mempool, we'll still be able to serve it from mapRelay.
This is still possible with mapRelay:
1. spy sends txA. It's adde
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191600264)
What I mean is
Leak available without mapRelay:
1. spy sends txA. It's added to mempool.
2. spy sends txB (through the network) and continuously queries our node for txA
3. once we accept txB to mempool, we serve notfound for txA (because it's not there anymore). spy learns exactly what time we received txB.
^This is prevented with mapRelay because after txA leaves mempool, we'll still be able to serve it from mapRelay.
This is still possible with mapRelay:
1. spy sends txA. It's adde
...
💬 glozow commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544581150)
What is the benefit of `mapRelay` overlapping with mempool? If we moved it into txmempool, could we just add things to `mapRelay` from `mapTx` whenever evicting something whose timestamp is recent? If the main reason is transactions that could be in blocks, it seems reasonable to cap it at just a few thousand entries?
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544581150)
What is the benefit of `mapRelay` overlapping with mempool? If we moved it into txmempool, could we just add things to `mapRelay` from `mapTx` whenever evicting something whose timestamp is recent? If the main reason is transactions that could be in blocks, it seems reasonable to cap it at just a few thousand entries?
💬 achow101 commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1191628983)
CI Failure here:
```
C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/src/test/txpackage_tests.cpp(622): error: in "txpackage_tests/package_submission_tests": check GetVirtualTransactionSize(*tx_parent2) == 191 has failed [190 != 191]
```
I'm going to guess that this is because real signatures are being created, which can occasionally be smaller than 71 bytes, thus resulting in a tx that is one byte smaller. If the script type doesn't matter, I would suggest using tapro
...
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1191628983)
CI Failure here:
```
C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/src/test/txpackage_tests.cpp(622): error: in "txpackage_tests/package_submission_tests": check GetVirtualTransactionSize(*tx_parent2) == 191 has failed [190 != 191]
```
I'm going to guess that this is because real signatures are being created, which can occasionally be smaller than 71 bytes, thus resulting in a tx that is one byte smaller. If the script type doesn't matter, I would suggest using tapro
...
💬 kroese commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1544593209)
I'm also seeing the same issue.. The load average suddenly increased from 0.25 to 1+. From the graph its clear it started on the 6th of May:
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1544593209)
I'm also seeing the same issue.. The load average suddenly increased from 0.25 to 1+. From the graph its clear it started on the 6th of May:
💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1191631520)
I'm fine if the magic number checks have a bit of wiggle room tbh, just leave a comment?
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1191631520)
I'm fine if the magic number checks have a bit of wiggle room tbh, just leave a comment?
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191650786)
if the issue is during warmup, that's a pretty small window, and typically won't actually do parallel fetching, 3 seems rare even when warmed up with limit of 10. I'll gather some more stats on that front, seeing how many parallel blocks are initialized.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191650786)
if the issue is during warmup, that's a pretty small window, and typically won't actually do parallel fetching, 3 seems rare even when warmed up with limit of 10. I'll gather some more stats on that front, seeing how many parallel blocks are initialized.
💬 furszy commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1544648748)
Thanks for the deep review @ryanofsky!
> Reviewed [86752e0](https://github.com/bitcoin/bitcoin/commit/86752e0cc5bc48f3d4ac1cd07835c37daf078d6a) and it mostly looks good, but does have a null pointer deference currently (see comments and suggested fix). I like this PR because it will simplify #24230 and could potentially improve performance. I'm not actually how sure how much it actually would improve performance in practice though, so I'm curious if that's was the original motivation here or
...
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1544648748)
Thanks for the deep review @ryanofsky!
> Reviewed [86752e0](https://github.com/bitcoin/bitcoin/commit/86752e0cc5bc48f3d4ac1cd07835c37daf078d6a) and it mostly looks good, but does have a null pointer deference currently (see comments and suggested fix). I like this PR because it will simplify #24230 and could potentially improve performance. I'm not actually how sure how much it actually would improve performance in practice though, so I'm curious if that's was the original motivation here or
...
💬 TheBlueMatt commented on issue "One core in CPU usage rate remains at 100% for a long time, causing serious delays in new blocks and forks":
(https://github.com/bitcoin/bitcoin/issues/27623#issuecomment-1544664280)
Are you not running any code to submit blocks across multiple bitcoin daemons quickly such as redundant nodes using the old FIBRE patchset?
(https://github.com/bitcoin/bitcoin/issues/27623#issuecomment-1544664280)
Are you not running any code to submit blocks across multiple bitcoin daemons quickly such as redundant nodes using the old FIBRE patchset?
💬 furszy commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#discussion_r1191686510)
It is true while you use the db handler only for reading at that point. In that case, you don't need the transaction argument and `batch.txn()` returns a `nullptr`.
If you mix an already open read cursor with some write operations, then the code is no longer performing a read-only operation, so it will need to provide the transaction arg (probably by re-arranging the code in the same way as did with `EraseRecords`).
(https://github.com/bitcoin/bitcoin/pull/27556#discussion_r1191686510)
It is true while you use the db handler only for reading at that point. In that case, you don't need the transaction argument and `batch.txn()` returns a `nullptr`.
If you mix an already open read cursor with some write operations, then the code is no longer performing a read-only operation, so it will need to provide the transaction arg (probably by re-arranging the code in the same way as did with `EraseRecords`).
💬 furszy commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#discussion_r1191688310)
Yeah, I explained the reason inside the commit description ca1871d48558b356257d2a95b046f63739dfe3a0:
> extra note from the Db.cursor doc:
"If transaction protection is enabled, cursors must be
opened and closed within the context of a transaction"
>
> thus why added a `CloseCursor` call before calling to
`TxnAbort/TxnCommit`.
The `CloseCursor` call is the ptr reset.
Just extra safety measures.
(https://github.com/bitcoin/bitcoin/pull/27556#discussion_r1191688310)
Yeah, I explained the reason inside the commit description ca1871d48558b356257d2a95b046f63739dfe3a0:
> extra note from the Db.cursor doc:
"If transaction protection is enabled, cursors must be
opened and closed within the context of a transaction"
>
> thus why added a `CloseCursor` call before calling to
`TxnAbort/TxnCommit`.
The `CloseCursor` call is the ptr reset.
Just extra safety measures.
💬 sr-gi commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544692524)
> > > > `txinfo.m_time <= mempool_req`
> >
> >
> > > This condition is checking that our inv (`txinfo`) is more recent than the last `mempool_req`, meaning that once a mempool request has been processed the peer can request any transaction from our mempool.
> >
> >
> > I don't get it. Is it not that the condition is checking whether `txinfo` is _older_ than the last `mempool_req`? In that case the transaction has been part of the `MEMPOOL` reply, so we have sent an `INV` about it.
>
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544692524)
> > > > `txinfo.m_time <= mempool_req`
> >
> >
> > > This condition is checking that our inv (`txinfo`) is more recent than the last `mempool_req`, meaning that once a mempool request has been processed the peer can request any transaction from our mempool.
> >
> >
> > I don't get it. Is it not that the condition is checking whether `txinfo` is _older_ than the last `mempool_req`? In that case the transaction has been part of the `MEMPOOL` reply, so we have sent an `INV` about it.
>
...
📝 theStack opened a pull request: "test: avoid sporadic MINIMALDATA failure in feature_taproot.py (fixes #27595)"
(https://github.com/bitcoin/bitcoin/pull/27631)
The functional test feature_taproot.py fails in some rare cases on the execution of the following `"branched_codesep"` spending script (can be reproduced via `$ ./test/functional/feature_taproot.py --randomseed 9048710178866422833` on master / 137a98c5a22e058ed7a7997a0a4dbd75301de51e):
https://github.com/bitcoin/bitcoin/blob/9d85c03620bf660cfa7d13080f5c0b191579cbc3/test/functional/feature_taproot.py#L741
The problem occurs if the first data-push (having random content with a random lengt
...
(https://github.com/bitcoin/bitcoin/pull/27631)
The functional test feature_taproot.py fails in some rare cases on the execution of the following `"branched_codesep"` spending script (can be reproduced via `$ ./test/functional/feature_taproot.py --randomseed 9048710178866422833` on master / 137a98c5a22e058ed7a7997a0a4dbd75301de51e):
https://github.com/bitcoin/bitcoin/blob/9d85c03620bf660cfa7d13080f5c0b191579cbc3/test/functional/feature_taproot.py#L741
The problem occurs if the first data-push (having random content with a random lengt
...
💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191715247)
Sure :+1: , fixed.
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191715247)
Sure :+1: , fixed.