🚀 fanquake merged a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125)
(https://github.com/bitcoin/bitcoin/pull/27125)
💬 kouloumos commented on issue "rpc: Allow importing wallets by data instead of by filename":
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1543668223)
I might be missing something here, as I cannot understand how your latest question is related to your initial feature request.
It is common for backups to be in files, hence importing/restoring from a file. I understand the over-the-wire scenario, but if there is no strong use-case for such a feature, it makes it more difficult to gather interest.
Note that `importwallet` and the related `dumpwallet` RPC are only compatible with legacy wallets, and as there is a [proposed timeline for their
...
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1543668223)
I might be missing something here, as I cannot understand how your latest question is related to your initial feature request.
It is common for backups to be in files, hence importing/restoring from a file. I understand the over-the-wire scenario, but if there is no strong use-case for such a feature, it makes it more difficult to gather interest.
Note that `importwallet` and the related `dumpwallet` RPC are only compatible with legacy wallets, and as there is a [proposed timeline for their
...
💬 fanquake commented on pull request "kernel: Remove args, chainparams, chainparamsbase from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1543685914)
```bash
src/common/settings.h seems to be missing the expected include guard:
#ifndef BITCOIN_COMMON_SETTINGS_H
#define BITCOIN_COMMON_SETTINGS_H
...
#endif // BITCOIN_COMMON_SETTINGS_H
```
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1543685914)
```bash
src/common/settings.h seems to be missing the expected include guard:
#ifndef BITCOIN_COMMON_SETTINGS_H
#define BITCOIN_COMMON_SETTINGS_H
...
#endif // BITCOIN_COMMON_SETTINGS_H
```
💬 ajtowns 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#discussion_r1190924614)
> But how about just recording the timestamp of the last MEMPOOL request ...
Patch might look something like...
<details><summary>Details</summary>
```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -295,6 +295,8 @@ struct Peer {
* permitted if the peer has NetPermissionFlags::Mempool or we advertise
* NODE_BLOOM. See BIP35. */
bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
+ std::chrono::microseconds m_last_memp
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190924614)
> But how about just recording the timestamp of the last MEMPOOL request ...
Patch might look something like...
<details><summary>Details</summary>
```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -295,6 +295,8 @@ struct Peer {
* permitted if the peer has NetPermissionFlags::Mempool or we advertise
* NODE_BLOOM. See BIP35. */
bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
+ std::chrono::microseconds m_last_memp
...
💬 MarcoFalke commented on issue "rpc: Allow importing wallets by data instead of by filename":
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1543700893)
Passing large data blobs, presumably multi-megabyte raw byte files, encoded as (presumably) hex, wrapped in json, over RPC is likely going to cause more issues than it fixes.
See also https://github.com/bitcoin/bitcoin/issues?q=%22Argument+list+too+long%22:
* GUI https://github.com/bitcoin/bitcoin/issues/17618
* Bash https://github.com/bitcoin/bitcoin/issues/4099
* Python https://github.com/bitcoin/bitcoin/issues/14767#issuecomment-619644381
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1543700893)
Passing large data blobs, presumably multi-megabyte raw byte files, encoded as (presumably) hex, wrapped in json, over RPC is likely going to cause more issues than it fixes.
See also https://github.com/bitcoin/bitcoin/issues?q=%22Argument+list+too+long%22:
* GUI https://github.com/bitcoin/bitcoin/issues/17618
* Bash https://github.com/bitcoin/bitcoin/issues/4099
* Python https://github.com/bitcoin/bitcoin/issues/14767#issuecomment-619644381
💬 jonatack commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1543705947)
Concept ACK. I've been adding sources files to this list while working on each pull.
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1543705947)
Concept ACK. I've been adding sources files to this list while working on each pull.
📝 fanquake opened a pull request: "[23.2] Backports for rc1"
(https://github.com/bitcoin/bitcoin/pull/27624)
Collecting backports for rc1. Currently:
* https://github.com/bitcoin/bitcoin/pull/27608 (not a clean cherry-pick)
(https://github.com/bitcoin/bitcoin/pull/27624)
Collecting backports for rc1. Currently:
* https://github.com/bitcoin/bitcoin/pull/27608 (not a clean cherry-pick)
💬 jonatack commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#discussion_r1190956000)
Good point; removed that commit. Thanks for having a look @fanquake.
(https://github.com/bitcoin/bitcoin/pull/27385#discussion_r1190956000)
Good point; removed that commit. Thanks for having a look @fanquake.
💬 ajtowns 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#discussion_r1190966758)
Okay, other idea: how about just making sure you only send at most 3500 mempool entries younger than 2 minutes, in response to a MEMPOOL message, and add them all to the filter, and don't worry if that causes overflow on results from a previous response?
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190966758)
Okay, other idea: how about just making sure you only send at most 3500 mempool entries younger than 2 minutes, in response to a MEMPOOL message, and add them all to the filter, and don't worry if that causes overflow on results from a previous response?
💬 willcl-ark 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-1543749307)
> i was under the impression we were thinking of the BIP35 as permissioned anyways.
> ...
> For instance BTCPay/NBXplorer needs `whitelist=mempool` iirc.
Just on these points, currently the MEMPOOL message will be responded to _either_ if you have the whitelist permission `mempool` on the peer **or** if you have enabled NODE_BLOOM (with `-peerbloomfilters`). In the latter case I don't think we can accurately describe these as "trusted peers". See the changes in [`4581a68` (#27559)](https://
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1543749307)
> i was under the impression we were thinking of the BIP35 as permissioned anyways.
> ...
> For instance BTCPay/NBXplorer needs `whitelist=mempool` iirc.
Just on these points, currently the MEMPOOL message will be responded to _either_ if you have the whitelist permission `mempool` on the peer **or** if you have enabled NODE_BLOOM (with `-peerbloomfilters`). In the latter case I don't think we can accurately describe these as "trusted peers". See the changes in [`4581a68` (#27559)](https://
...
💬 MarcoFalke 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#discussion_r1190981465)
> how about just making sure you only send at most 3500 mempool entries younger than 2 minutes, in response to a MEMPOOL message
Wouldn't that imply that peers are now expected to send the `mempool` message in a loop to detect if there are more transactions to get? If yes, I wonder if it conflicts with the idea to potentially only allow a single `mempool` message per connection.
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190981465)
> how about just making sure you only send at most 3500 mempool entries younger than 2 minutes, in response to a MEMPOOL message
Wouldn't that imply that peers are now expected to send the `mempool` message in a loop to detect if there are more transactions to get? If yes, I wonder if it conflicts with the idea to potentially only allow a single `mempool` message per connection.
💬 michaelfolkson commented on pull request "add ryanofsky to trusted-keys":
(https://github.com/bitcoin/bitcoin/pull/27604#issuecomment-1543765335)
Also for posterity: https://gist.github.com/michaelfolkson/81df14508ed3ad3232da133e154d6873
The maintainer role was always supposed to be janitorial one and was not elevated above the many long term contributors on the project. A hundred(s) billion dollar ecosystem shouldn't depend on the whims of a small group of maintainers who refuse to engage on decisions that not only impact the project but potentially the entire ecosystem (currently valued in the hundreds of billions of dollars). The fa
...
(https://github.com/bitcoin/bitcoin/pull/27604#issuecomment-1543765335)
Also for posterity: https://gist.github.com/michaelfolkson/81df14508ed3ad3232da133e154d6873
The maintainer role was always supposed to be janitorial one and was not elevated above the many long term contributors on the project. A hundred(s) billion dollar ecosystem shouldn't depend on the whims of a small group of maintainers who refuse to engage on decisions that not only impact the project but potentially the entire ecosystem (currently valued in the hundreds of billions of dollars). The fa
...
💬 dergoegge commented on pull request "p2p: Avoid prematurely clearing download state for other peers":
(https://github.com/bitcoin/bitcoin/pull/27608#issuecomment-1543775838)
post-merge Code review ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4
(https://github.com/bitcoin/bitcoin/pull/27608#issuecomment-1543775838)
post-merge Code review ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4
💬 stickies-v commented on pull request "add ryanofsky to trusted-keys":
(https://github.com/bitcoin/bitcoin/pull/27604#issuecomment-1543798949)
> The fact that I'm the only one here stating the obvious (...) is really scary to me.
Not to speak for anyone else, but perhaps the lack of engagement on your messages is because most regular contributors are just generally happy with the maintainer decisions and don't agree with your assessment but do not wish to keep discussing the same thing time and again?
(https://github.com/bitcoin/bitcoin/pull/27604#issuecomment-1543798949)
> The fact that I'm the only one here stating the obvious (...) is really scary to me.
Not to speak for anyone else, but perhaps the lack of engagement on your messages is because most regular contributors are just generally happy with the maintainer decisions and don't agree with your assessment but do not wish to keep discussing the same thing time and again?
💬 MarcoFalke 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#discussion_r1191023800)
I wonder if we can remove mapRelay and as a result can get more efficient data structures to remember (w)txids per `Peer`, knowing that only transaction in the mempool can be in them.
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191023800)
I wonder if we can remove mapRelay and as a result can get more efficient data structures to remember (w)txids per `Peer`, knowing that only transaction in the mempool can be in them.
💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1543830285)
With #27468 merged, I don't really see the need for 8ef940d39bfa7bc4ba867b70495b68c507297694 anymore?
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1543830285)
With #27468 merged, I don't really see the need for 8ef940d39bfa7bc4ba867b70495b68c507297694 anymore?
💬 fanquake commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1543831532)
> Shall I go ahead and PR that commit separately?
@theuni sgtm. I've got some related compile flag changes in #27493, but I'm going to just combine most of the relevant changes here. If we get the LLD switchover done for 26.x, then we can skip another bump in the interim.
For now, I've rebased this PR on top of all of your other commits above, and reverted back to LLVM 15.0.6 in depends. Also updated the PR description.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1543831532)
> Shall I go ahead and PR that commit separately?
@theuni sgtm. I've got some related compile flag changes in #27493, but I'm going to just combine most of the relevant changes here. If we get the LLD switchover done for 26.x, then we can skip another bump in the interim.
For now, I've rebased this PR on top of all of your other commits above, and reverted back to LLVM 15.0.6 in depends. Also updated the PR description.
💬 michaelfolkson commented on pull request "add ryanofsky to trusted-keys":
(https://github.com/bitcoin/bitcoin/pull/27604#issuecomment-1543835912)
> Not to speak for anyone else, but perhaps the lack of engagement on your messages is because most regular contributors are just generally happy with the maintainer decisions and don't agree with your assessment but do not wish to keep discussing the same thing time and again?
It is a failure of communication and process around both CTV (which could have resulted in a chain split disrupting a hundred(s) billion dollar ecosystem) and now the addition, rejection and removal of maintainers. If
...
(https://github.com/bitcoin/bitcoin/pull/27604#issuecomment-1543835912)
> Not to speak for anyone else, but perhaps the lack of engagement on your messages is because most regular contributors are just generally happy with the maintainer decisions and don't agree with your assessment but do not wish to keep discussing the same thing time and again?
It is a failure of communication and process around both CTV (which could have resulted in a chain split disrupting a hundred(s) billion dollar ecosystem) and now the addition, rejection and removal of maintainers. If
...
💬 theStack commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1191062084)
This field is set in `GetPrioritisedTransaction`, but not read anywhere. Was there a plan to also return that in `getprioritisedtransactions` or is there any other future use-case? If not, I think it can be removed.
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1191062084)
This field is set in `GetPrioritisedTransaction`, but not read anywhere. Was there a plan to also return that in `getprioritisedtransactions` or is there any other future use-case? If not, I think it can be removed.
💬 vasild 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-1543917289)
The title "avoid serving non-announced txs as a result of a MEMPOOL message" is confusing - it gives the impression that this PR will change the response to `MEMPOOL` messages, but it does not do that. Before and after the PR we would send the full mempool in a reply to `mempool` (correct me if I got it wrong).
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1543917289)
The title "avoid serving non-announced txs as a result of a MEMPOOL message" is confusing - it gives the impression that this PR will change the response to `MEMPOOL` messages, but it does not do that. Before and after the PR we would send the full mempool in a reply to `mempool` (correct me if I got it wrong).