💬 mzumsande commented on pull request "indexes: Read the locator's top block during init, allow interaction with reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/25193#discussion_r1191521866)
That's correct! I didn't want to change the db format though to not break compatibility.
(https://github.com/bitcoin/bitcoin/pull/25193#discussion_r1191521866)
That's correct! I didn't want to change the db format though to not break compatibility.
💬 mzumsande commented on pull request "indexes: Read the locator's top block during init, allow interaction with reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/25193#issuecomment-1544447815)
Thanks! I'll rebase and address furszy's comments next week!
(https://github.com/bitcoin/bitcoin/pull/25193#issuecomment-1544447815)
Thanks! I'll rebase and address furszy's comments next week!
💬 dergoegge commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191523726)
Imo, it doesn't seem worthwhile to have the limit:
* the good case (small K) should be far more common than the bad one (large K)
* the limit makes relay of blocks with large K less reliable w.r.t to stalling (reduces to what we have now)
* the limit adds code complexity and a maintenance burden. 10 is not a good value according to your data and if #10984 had been merged then we would need to re-evaluate now. Who's to say this won't change again.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191523726)
Imo, it doesn't seem worthwhile to have the limit:
* the good case (small K) should be far more common than the bad one (large K)
* the limit makes relay of blocks with large K less reliable w.r.t to stalling (reduces to what we have now)
* the limit adds code complexity and a maintenance burden. 10 is not a good value according to your data and if #10984 had been merged then we would need to re-evaluate now. Who's to say this won't change again.
🚀 fanquake merged a pull request: "[25.0] Backports for rc2"
(https://github.com/bitcoin/bitcoin/pull/27613)
(https://github.com/bitcoin/bitcoin/pull/27613)
💬 dergoegge commented on pull request "[23.2] Backports for rc1":
(https://github.com/bitcoin/bitcoin/pull/27624#issuecomment-1544470038)
ACK a26ff204f0f0355749a1b61136437623b325f8fb
(https://github.com/bitcoin/bitcoin/pull/27624#issuecomment-1544470038)
ACK a26ff204f0f0355749a1b61136437623b325f8fb
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191541721)
after attempting the change, I think that would violate https://github.com/bitcoin/bitcoin/pull/27626/commits/e424dcba60e61f014b19a3e2f59238ca3b91f3db#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1394 which requires insertion ordering to know which is the stalling peer.
totally forgot about the FIXME...
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191541721)
after attempting the change, I think that would violate https://github.com/bitcoin/bitcoin/pull/27626/commits/e424dcba60e61f014b19a3e2f59238ca3b91f3db#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1394 which requires insertion ordering to know which is the stalling peer.
totally forgot about the FIXME...
💬 MarcoFalke commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1191549860)
```
rpc/request.cpp:212:14: error: the variable 'valJsonRPC' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [performance-unnecessary-copy-initialization,-warnings-as-errors]
UniValue valJsonRPC = request.find_value("jsonrpc");
^
const &
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1191549860)
```
rpc/request.cpp:212:14: error: the variable 'valJsonRPC' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [performance-unnecessary-copy-initialization,-warnings-as-errors]
UniValue valJsonRPC = request.find_value("jsonrpc");
^
const &
💬 MarcoFalke commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1544495872)
I presume this is still an issue? If yes, what about filing a new one, given that this issue is about `--enable-debug` (not `-debug`), and most comments here may be about a recently fixed issue #https://github.com/bitcoin/bitcoin/issues/27623 and not about `--enable-debug` performance?
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1544495872)
I presume this is still an issue? If yes, what about filing a new one, given that this issue is about `--enable-debug` (not `-debug`), and most comments here may be about a recently fixed issue #https://github.com/bitcoin/bitcoin/issues/27623 and not about `--enable-debug` performance?
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191560489)
ok, I can see that and how it reads cleaner, changed
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191560489)
ok, I can see that and how it reads cleaner, changed
💬 MarcoFalke commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191566131)
I think you forgot to remove the sleep?
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191566131)
I think you forgot to remove the sleep?
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191569820)
done, cleaner thank you
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191569820)
done, cleaner thank you
💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191573164)
My bad, thanks.
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191573164)
My bad, thanks.
🤔 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?