💬 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.
🚀 fanquake merged a pull request: "validation: Move warningcache to ChainstateManager and rename to m_warningcache"
(https://github.com/bitcoin/bitcoin/pull/27357)
(https://github.com/bitcoin/bitcoin/pull/27357)
💬 stickies-v commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226635021)
Why do we need this repeated test? At the very least it seems orthogonal to the PR?
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226635021)
Why do we need this repeated test? At the very least it seems orthogonal to the PR?
💬 stickies-v commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226624400)
We don't do any named argument transformation when calling `getdeploymentinfo().HandleRequest(jsonRequest)` directly, so the `jsonRequest` is passed as is. If you look at the implementation of `getdeploymentinfo()`, you'll see it fetches the argument by position: `request.params[0]`.
Even though this implementation works, it's pretty fragile: it would also work if you'd used `jsonRequest.params.pushKV("not-a-blockhash", hash_str);`, for example.
I think sticking to a `VARR` is best:
```
...
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226624400)
We don't do any named argument transformation when calling `getdeploymentinfo().HandleRequest(jsonRequest)` directly, so the `jsonRequest` is passed as is. If you look at the implementation of `getdeploymentinfo()`, you'll see it fetches the argument by position: `request.params[0]`.
Even though this implementation works, it's pretty fragile: it would also work if you'd used `jsonRequest.params.pushKV("not-a-blockhash", hash_str);`, for example.
I think sticking to a `VARR` is best:
```
...
⚠️ fanquake opened an issue: "tsan: failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/issues/27860)
master @ 6f5f37eefd425faabd9a97a3c3028395c34f08c4.
https://cirrus-ci.com/task/5086176751648768?logs=ci#L3409:
```bash
node0 2023-06-12T12:48:24.904003Z [msghand] [txmempool.cpp:1011] [RemoveUnbroadcastTx] [mempool] Removed 0668eb571427ae4ed5ee1f9d9d3d0c6de4261bba44335d033a75be0183d87a46 from set of unbroadcast txns
test 2023-06-12T12:48:24.952000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
...
(https://github.com/bitcoin/bitcoin/issues/27860)
master @ 6f5f37eefd425faabd9a97a3c3028395c34f08c4.
https://cirrus-ci.com/task/5086176751648768?logs=ci#L3409:
```bash
node0 2023-06-12T12:48:24.904003Z [msghand] [txmempool.cpp:1011] [RemoveUnbroadcastTx] [mempool] Removed 0668eb571427ae4ed5ee1f9d9d3d0c6de4261bba44335d033a75be0183d87a46 from set of unbroadcast txns
test 2023-06-12T12:48:24.952000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
...
💬 dergoegge commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587311912)
> 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
What are you thinking of in terms of flexibility that the current interfaces do not offer? I can't think of anything that would not work by using the current interfaces or exposing extra functionality through them. For example, looking at Matt's comment https://github.com/bitcoin/bitcoin/pull/23049#issuecomment-926009122:
> do things l
...
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587311912)
> 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
What are you thinking of in terms of flexibility that the current interfaces do not offer? I can't think of anything that would not work by using the current interfaces or exposing extra functionality through them. For example, looking at Matt's comment https://github.com/bitcoin/bitcoin/pull/23049#issuecomment-926009122:
> do things l
...