💬 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.
🚀 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):
...