💬 glozow commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1544440056)
> IsChildWithParentsTree <---- is this originally what the V3 topo looked like?
Yep exactly. 1 child multiple parents. Parent's can't spend each other. You can use this for batch-bumping commitment transactions, for example.
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1544440056)
> IsChildWithParentsTree <---- is this originally what the V3 topo looked like?
Yep exactly. 1 child multiple parents. Parent's can't spend each other. You can use this for batch-bumping commitment transactions, for example.
💬 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
...