📝 ismaelsadeeq opened a pull request: "Mempool: persist mempoolminfee accross restarts"
(https://github.com/bitcoin/bitcoin/pull/27859)
Follow-up [#27622](https://github.com/bitcoin/bitcoin/pull/27622) depends on [#27622](https://github.com/bitcoin/bitcoin/pull/27622)
See –> [Comment](https://github.com/bitcoin/bitcoin/issues/27555#issuecomment-1538504504)
Currently, there is no persistence of mempoolminfee
As mentioned in the comment
> persist `mempoolminfee` across restarts to at least tell user a plausible value to stay in the mempool for a bit
The suggestions outlined in the comment are implemented in this pull
...
(https://github.com/bitcoin/bitcoin/pull/27859)
Follow-up [#27622](https://github.com/bitcoin/bitcoin/pull/27622) depends on [#27622](https://github.com/bitcoin/bitcoin/pull/27622)
See –> [Comment](https://github.com/bitcoin/bitcoin/issues/27555#issuecomment-1538504504)
Currently, there is no persistence of mempoolminfee
As mentioned in the comment
> persist `mempoolminfee` across restarts to at least tell user a plausible value to stay in the mempool for a bit
The suggestions outlined in the comment are implemented in this pull
...
💬 MarcoFalke commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1226387994)
Might be better to write this to `mempool.dat`, or is there a benefit to create a new file?
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1226387994)
Might be better to write this to `mempool.dat`, or is there a benefit to create a new file?
💬 ajtowns commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1226384032)
Why add this as a separate file, rather than adding it to the end of the fee estimates data file which is already updated periodically and ignored if out of date?
Adding a method to the mempool to set the min fee based on that seems plausible? (Perhaps make it conditional on the new value being higher than the current minfee, and the mempool being less than 95% full?)
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1226384032)
Why add this as a separate file, rather than adding it to the end of the fee estimates data file which is already updated periodically and ignored if out of date?
Adding a method to the mempool to set the min fee based on that seems plausible? (Perhaps make it conditional on the new value being higher than the current minfee, and the mempool being less than 95% full?)
💬 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
...