💬 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.
💬 sipa commented on pull request "test: avoid sporadic MINIMALDATA failure in feature_taproot.py (fixes #27595)":
(https://github.com/bitcoin/bitcoin/pull/27631#issuecomment-1544738994)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27631#issuecomment-1544738994)
Concept ACK
🤔 stickies-v reviewed a pull request: "net: Allow inbound whitebind connections to more aggresivey evict peers when slots are full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1423467785)
Concept ACK, makes sense to prioritize whitelisted peers. Will review more in-depth soon.
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1423467785)
Concept ACK, makes sense to prioritize whitelisted peers. Will review more in-depth soon.
💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggresivey evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1191709646)
nit
```suggestion
return force ? std::make_optional(last_out.id) : std::nullopt;
```
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1191709646)
nit
```suggestion
return force ? std::make_optional(last_out.id) : std::nullopt;
```
💬 jonatack commented on pull request "net: Allow inbound whitebind connections to more aggresivey evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1191732820)
Not sure, but IIRC last time I looked RVO wasn't working with a ternary.
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1191732820)
Not sure, but IIRC last time I looked RVO wasn't working with a ternary.
💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1544783273)
Thanks @Sjors. I've tried a lot of things and have not been able to reproduce the crash exactly, but the log you posted was helpful, and I could reproduce similar error output by adding an extra call from bitcoin-node to bitcoin-wallet that sleeps in bitcoin-wallet, and then manual killing `bitcoin-wallet` process. When I do that I see similar `kj::Exception` errors on the bitcoin-node main thread and bitcoin-node b-capnp-loop thread like in your log. In your log the exact errors are `kj/async
...
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-1544783273)
Thanks @Sjors. I've tried a lot of things and have not been able to reproduce the crash exactly, but the log you posted was helpful, and I could reproduce similar error output by adding an extra call from bitcoin-node to bitcoin-wallet that sleeps in bitcoin-wallet, and then manual killing `bitcoin-wallet` process. When I do that I see similar `kj::Exception` errors on the bitcoin-node main thread and bitcoin-node b-capnp-loop thread like in your log. In your log the exact errors are `kj/async
...
📝 jonatack opened a pull request: "Raise on invalid -debug and -loglevel config options"
(https://github.com/bitcoin/bitcoin/pull/27632)
and rename BCLog::BLOCKSTORE to BLOCKSTORAGE so the enum is the same as its value like the other BCLog enums.
Per discussion in bitcoin-core-dev IRC today from https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-05-11#921458.
(https://github.com/bitcoin/bitcoin/pull/27632)
and rename BCLog::BLOCKSTORE to BLOCKSTORAGE so the enum is the same as its value like the other BCLog enums.
Per discussion in bitcoin-core-dev IRC today from https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-05-11#921458.
💬 amitiuttarwar commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191790448)
@satsie is the concern of having a dummy value like `NET_MAX` that it could fall out of
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191790448)
@satsie is the concern of having a dummy value like `NET_MAX` that it could fall out of
🤔 amitiuttarwar reviewed a pull request: "rpc: add 'getnetmsgstats', new rpc to view network message statistics"
(https://github.com/bitcoin/bitcoin/pull/27534#pullrequestreview-1423599288)
approach ACK. did a high-level review and left a few small comments
(https://github.com/bitcoin/bitcoin/pull/27534#pullrequestreview-1423599288)
approach ACK. did a high-level review and left a few small comments
💬 amitiuttarwar commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191793368)
this function should handle if `index > NUM_NET_MESSAGE_TYPES`
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191793368)
this function should handle if `index > NUM_NET_MESSAGE_TYPES`
💬 amitiuttarwar commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191798171)
nit: I'm kinda confused by this spacing. could it be a bit more legible to have them just line up instead of cascade?
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191798171)
nit: I'm kinda confused by this spacing. could it be a bit more legible to have them just line up instead of cascade?
💬 amitiuttarwar commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191801732)
cc @dergoegge
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191801732)
cc @dergoegge
💬 amitiuttarwar commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191790800)
@satsie is the concern of having a dummy value like `NET_MAX` that it could fall out of sync if enum values start being explicitly assigned?
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1191790800)
@satsie is the concern of having a dummy value like `NET_MAX` that it could fall out of sync if enum values start being explicitly assigned?