💬 ajtowns commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1226391232)
The idea is to update it periodically, and writing the full mempool to disk regularly may be unappealing.
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1226391232)
The idea is to update it periodically, and writing the full mempool to disk regularly may be unappealing.
🚀 fanquake merged a pull request: "p2p: Stop relaying non-mempool txs"
(https://github.com/bitcoin/bitcoin/pull/27625)
(https://github.com/bitcoin/bitcoin/pull/27625)
💬 dergoegge commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1586988190)
What are the reasons for not having this as a separate piece of software that calls the Bitcoin Core RPCs? From glancing at the code here it seems like `getblocktemplate` and `submitblock` would be the only needed RPCs. I've seen the discussion about separation (multi-process) in https://github.com/bitcoin/bitcoin/pull/23049 but wasn't convinced by the arguments there (e.g. https://github.com/bitcoin/bitcoin/pull/23049#issuecomment-926009122). I am worried about the maintenance burden of this an
...
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1586988190)
What are the reasons for not having this as a separate piece of software that calls the Bitcoin Core RPCs? From glancing at the code here it seems like `getblocktemplate` and `submitblock` would be the only needed RPCs. I've seen the discussion about separation (multi-process) in https://github.com/bitcoin/bitcoin/pull/23049 but wasn't convinced by the arguments there (e.g. https://github.com/bitcoin/bitcoin/pull/23049#issuecomment-926009122). I am worried about the maintenance burden of this an
...
💬 hebasto commented on pull request "Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1586988860)
Rebased b023c26eb8eda4c7f80ad2e7ebe1fb046e87d2ee -> e89273e80765219ff545ee5c26a257aef7d69f9c ([pr26762.10](https://github.com/hebasto/bitcoin/commits/pr26762.10) -> [pr26762.11](https://github.com/hebasto/bitcoin/commits/pr26762.11)) due to the conflict with #27576.
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1586988860)
Rebased b023c26eb8eda4c7f80ad2e7ebe1fb046e87d2ee -> e89273e80765219ff545ee5c26a257aef7d69f9c ([pr26762.10](https://github.com/hebasto/bitcoin/commits/pr26762.10) -> [pr26762.11](https://github.com/hebasto/bitcoin/commits/pr26762.11)) due to the conflict with #27576.
💬 dergoegge commented on pull request "Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1587015221)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1587015221)
Concept ACK
💬 MarcoFalke commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1226417224)
(This was in reply to my question why not to save it to `mempool.dat`, which was accidentally deleted)
> writing the full mempool to disk regularly may be unappealing
Probably off-topic here, but if we assume that the fee estimates must be written regularly due to unclean shutdowns, then the mempool should be as well, no?
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1226417224)
(This was in reply to my question why not to save it to `mempool.dat`, which was accidentally deleted)
> writing the full mempool to disk regularly may be unappealing
Probably off-topic here, but if we assume that the fee estimates must be written regularly due to unclean shutdowns, then the mempool should be as well, no?
💬 Tguntenaar commented on pull request "contrib: docs fix --import-keys flag on verify.py":
(https://github.com/bitcoin/bitcoin/pull/27840#issuecomment-1587035148)
"to avoid an @ mention in the commit message." What does that mean?
(https://github.com/bitcoin/bitcoin/pull/27840#issuecomment-1587035148)
"to avoid an @ mention in the commit message." What does that mean?
💬 Fi3 commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587095727)
One reason is that `getblocktemplate` require polling bitcoind where `NewTemplate` will be pushed by bitcoind. What you call stratumV3 would likely be an extension of Sv2 and will likley use the same binary data format and framing of Sv2.
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587095727)
One reason is that `getblocktemplate` require polling bitcoind where `NewTemplate` will be pushed by bitcoind. What you call stratumV3 would likely be an extension of Sv2 and will likley use the same binary data format and framing of Sv2.
💬 dergoegge commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587116056)
> One reason is that getblocktemplate require polling bitcoind where NewTemplate will be pushed by bitcoind.
I'm imagining a separate process `stratumv2d` that implements the server from this PR but polls the block template through `getblocktemplate` instead of using the BlockAssembler directly (which is equivalent to a poll, minus the rpc roundtrip). stratumv2d could then still push template updates after polling from bitcoind.
> What you call stratumV3 would likely be an extension of Sv2
...
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587116056)
> One reason is that getblocktemplate require polling bitcoind where NewTemplate will be pushed by bitcoind.
I'm imagining a separate process `stratumv2d` that implements the server from this PR but polls the block template through `getblocktemplate` instead of using the BlockAssembler directly (which is equivalent to a poll, minus the rpc roundtrip). stratumv2d could then still push template updates after polling from bitcoind.
> What you call stratumV3 would likely be an extension of Sv2
...
💬 TheCharlatan commented on pull request "kernel: Remove shutdown globals from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1587127075)
Re https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869
> If you want to take time to iterate more on this code more, I think we could easily pull out parts of this PR right now that are pure improvements and could be reviewed and merged quickly.
Ok, I will put this PR back to draft, and open two new ones as you suggested. I'll continue using this draft to stage my further improvements.
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1587127075)
Re https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869
> If you want to take time to iterate more on this code more, I think we could easily pull out parts of this PR right now that are pure improvements and could be reviewed and merged quickly.
Ok, I will put this PR back to draft, and open two new ones as you suggested. I'll continue using this draft to stage my further improvements.
💬 willcl-ark commented on issue "ThreadI2PAcceptIncoming temporarily bypasses 125 connection ceiling":
(https://github.com/bitcoin/bitcoin/issues/27843#issuecomment-1587128836)
I can't test this right now, as I uninstalled i2pd and now don't seem to be able to re-connect to any of the reseed servers and get connected :(
A cursory look at the code reads to me that they should behave similarly; although as you state non-I2P connections are done in `AcceptConnection`, the check on open connections is actually in `CreateNodeFromAcceptedSocket`, which is called from both sites, i.e. also from `ThreadI2PAcceptIncoming`:
https://github.com/bitcoin/bitcoin/blob/fbe48f97d
...
(https://github.com/bitcoin/bitcoin/issues/27843#issuecomment-1587128836)
I can't test this right now, as I uninstalled i2pd and now don't seem to be able to re-connect to any of the reseed servers and get connected :(
A cursory look at the code reads to me that they should behave similarly; although as you state non-I2P connections are done in `AcceptConnection`, the check on open connections is actually in `CreateNodeFromAcceptedSocket`, which is called from both sites, i.e. also from `ThreadI2PAcceptIncoming`:
https://github.com/bitcoin/bitcoin/blob/fbe48f97d
...
💬 Fi3 commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587152706)
> I'm imagining a separate process stratumv2d that implements the server from this PR but polls the block template through getblocktemplate instead of using the BlockAssembler directly (which is equivalent to a poll, minus the rpc roundtrip). stratumv2d could then still push template updates after polling from bitcoind.
having it implemented into bitcoind is not a big change and will allow more flexibility in when templates are pushed respect to just poll `getblocktemplate`
> I was just t
...
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587152706)
> I'm imagining a separate process stratumv2d that implements the server from this PR but polls the block template through getblocktemplate instead of using the BlockAssembler directly (which is equivalent to a poll, minus the rpc roundtrip). stratumv2d could then still push template updates after polling from bitcoind.
having it implemented into bitcoind is not a big change and will allow more flexibility in when templates are pushed respect to just poll `getblocktemplate`
> I was just t
...
💬 Crypt-iQ commented on issue "ThreadI2PAcceptIncoming temporarily bypasses 125 connection ceiling":
(https://github.com/bitcoin/bitcoin/issues/27843#issuecomment-1587159335)
The connections will get marked for disconnect in AttemptToEvictConnection but linger until DisconnectNodes is called. I was calling the `getconnectioncount` RPC, so these are connections that bitcoind has. Again not sure if this matters as I couldn't exhaust file descriptors, but figured I would point it out since the original behavior was to:
- connect
- mark for disconnect
- disconnect
all in the same thread.
(https://github.com/bitcoin/bitcoin/issues/27843#issuecomment-1587159335)
The connections will get marked for disconnect in AttemptToEvictConnection but linger until DisconnectNodes is called. I was calling the `getconnectioncount` RPC, so these are connections that bitcoind has. Again not sure if this matters as I couldn't exhaust file descriptors, but figured I would point it out since the original behavior was to:
- connect
- mark for disconnect
- disconnect
all in the same thread.
💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226531718)
done
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226531718)
done
💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226531889)
done
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226531889)
done
👋 ajtowns's pull request is ready for review: "net_processing: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675)
(https://github.com/bitcoin/bitcoin/pull/27675)
💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1587176585)
@brunoerg Thanks for the review! I like the idea of using the scheduler and I applied that change. However, I am unsure about tying the check to adding new addresses to the new table. It seems arbitrary to put it there, I would also like to run it on removals if we were going this route. From looking at some debug logs from a node I resynced the day before, I see changes in the address tables happening every couple of minutes. So we would still need to track the elapsed time to not run the check
...
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1587176585)
@brunoerg Thanks for the review! I like the idea of using the scheduler and I applied that change. However, I am unsure about tying the check to adding new addresses to the new table. It seems arbitrary to put it there, I would also like to run it on removals if we were going this route. From looking at some debug logs from a node I resynced the day before, I see changes in the address tables happening every couple of minutes. So we would still need to track the elapsed time to not run the check
...
💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1587182546)
> > [it is trivial for every connection to probe data on the node's mempool] already the case for any txs that have been in the mempool for greater than two minutes, and any transactions that have been announced to a peer. Generally that's plenty for fingerprinting -- if you announce pairs of conflicting txs randomly to different nodes, then if you do that 20 times at the same low-ish feerate (so that neither will rbf the other), then wait two minutes, you'll likely already get a nearly unique
...
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1587182546)
> > [it is trivial for every connection to probe data on the node's mempool] already the case for any txs that have been in the mempool for greater than two minutes, and any transactions that have been announced to a peer. Generally that's plenty for fingerprinting -- if you announce pairs of conflicting txs randomly to different nodes, then if you do that 20 times at the same low-ish feerate (so that neither will rbf the other), then wait two minutes, you'll likely already get a nearly unique
...
💬 MarcoFalke commented on issue "CI failure: `ThreadSanitizer: data race /usr/lib/llvm-12/bin/../include/c++/v1/ios:523:12 in std::__1::ios_base::width() const`":
(https://github.com/bitcoin/bitcoin/issues/23366#issuecomment-1587218489)
Maybe we can set https://en.cppreference.com/w/cpp/io/ios_base/sync_with_stdio as a work-around to get thread-safety?
(https://github.com/bitcoin/bitcoin/issues/23366#issuecomment-1587218489)
Maybe we can set https://en.cppreference.com/w/cpp/io/ios_base/sync_with_stdio as a work-around to get thread-safety?
💬 fanquake commented on pull request "validation: Move warningcache to ChainstateManager and rename to m_warningcache":
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1587230845)
I think it's ok to merge this now.
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1587230845)
I think it's ok to merge this now.