💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2384295168)
Sorry for the repeated force pushes; I made a change in the wrong commit, and then made some minor improvements to the new fuzz too.
Absent non-nitty review that needs addressing, I don't intend to make change here anymore.
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2384295168)
Sorry for the repeated force pushes; I made a change in the wrong commit, and then made some minor improvements to the new fuzz too.
Absent non-nitty review that needs addressing, I don't intend to make change here anymore.
💬 iotamega commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384325909)
> @jonatack While we should always be vigilant about the possibility of contributors with bad intentions, I think it is more productive to do so by judging contributions by their merit than by trying to infer people's intent.
I think intent here is pretty clear considering I am now working with CloudFlare to stop a DDOS attack on all of my sites.
I asked for this because of a friend in a country where they are having issues, very real issues and they did not want to doxx themselves but I a
...
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384325909)
> @jonatack While we should always be vigilant about the possibility of contributors with bad intentions, I think it is more productive to do so by judging contributions by their merit than by trying to infer people's intent.
I think intent here is pretty clear considering I am now working with CloudFlare to stop a DDOS attack on all of my sites.
I asked for this because of a friend in a country where they are having issues, very real issues and they did not want to doxx themselves but I a
...
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781704314)
nit: same here, I'd remove "all"
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781704314)
nit: same here, I'd remove "all"
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781795191)
I've been thinking about leaking information about private broadcast via addrman:
1.) Here, we call `Good()`, possibly moving the peer from new to tried. Since tried table is smaller and sparsely filled especially for newer nodes, and addrs are picked with 50% probability from new/tried, this usually means that we are more likely to connect to them in the future - maybe via clearnet, maybe via future private connections.
2.) On disconnection, we call `Connected()`, changing the `nTime` of an a
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781795191)
I've been thinking about leaking information about private broadcast via addrman:
1.) Here, we call `Good()`, possibly moving the peer from new to tried. Since tried table is smaller and sparsely filled especially for newer nodes, and addrs are picked with 50% probability from new/tried, this usually means that we are more likely to connect to them in the future - maybe via clearnet, maybe via future private connections.
2.) On disconnection, we call `Connected()`, changing the `nTime` of an a
...
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781677003)
I would find the ThreadOpenConnection logic (which is already convoluted in master) easier to understand if the private broadcast related logic would be separated more clearly, e.g. this whole thing could be behind some bool `private_broadcast_enabled` so `m_private_broadcast.ShouldOpen` wouldn't be called in normal mode and one wouldn't need to scroll down and verify that this function does nothing then. Of course there is no functional difference, so it may be a matter of taste.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781677003)
I would find the ThreadOpenConnection logic (which is already convoluted in master) easier to understand if the private broadcast related logic would be separated more clearly, e.g. this whole thing could be behind some bool `private_broadcast_enabled` so `m_private_broadcast.ShouldOpen` wouldn't be called in normal mode and one wouldn't need to scroll down and verify that this function does nothing then. Of course there is no functional difference, so it may be a matter of taste.
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781868981)
Why store by `TxId` instead of `WtxId`? Would this add some attack surface, e.g. an attacker with many connections to peers receiving a private tx, and then sending each of their peers a modified version of the same tx with invalid witness data (but same TxId), in the hope of connecting to the originating node and clearing any further attempts to broadcast the transaction (node that `m_tx_for_private_broadcast.Remove()` also happens for invalid txs becaust it's done before validation)
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781868981)
Why store by `TxId` instead of `WtxId`? Would this add some attack surface, e.g. an attacker with many connections to peers receiving a private tx, and then sending each of their peers a modified version of the same tx with invalid witness data (but same TxId), in the hope of connecting to the originating node and clearing any further attempts to broadcast the transaction (node that `m_tx_for_private_broadcast.Remove()` also happens for invalid txs becaust it's done before validation)
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781701897)
nit: all peers for which tx relay is enabled (so not blocks-only, feelers etc.).
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781701897)
nit: all peers for which tx relay is enabled (so not blocks-only, feelers etc.).
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781381298)
I found this help text hard to parse with all the "/" (had to read it multiple times to understand it). I'm not sure if the rpc help is the best place to give motivation for why features exist (instead of just describing them), but maybe this could be a bit more succinct?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781381298)
I found this help text hard to parse with all the "/" (had to read it multiple times to understand it). I'm not sure if the rpc help is the best place to give motivation for why features exist (instead of just describing them), but maybe this could be a bit more succinct?
💬 cobratbq commented on pull request "doc: external-signer, example 'createwallet' RPC call requires eight argument":
(https://github.com/bitcoin/bitcoin/pull/30354#issuecomment-2384385184)
In addition: documentation is completely lacking for flags `--stdin` and which stdin-commands to expect, including their format. Additionally, if we passing in to `stdin` directly, why wouldn't we pass the PSBT in binary form? Just have `signtx ` prefix then the binary data. (I guess this is too late now, but there is no reasoning or documentation about this anywhere in the spec.)
(https://github.com/bitcoin/bitcoin/pull/30354#issuecomment-2384385184)
In addition: documentation is completely lacking for flags `--stdin` and which stdin-commands to expect, including their format. Additionally, if we passing in to `stdin` directly, why wouldn't we pass the PSBT in binary form? Just have `signtx ` prefix then the binary data. (I guess this is too late now, but there is no reasoning or documentation about this anywhere in the spec.)
💬 jonatack commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384388036)
> it holds significant promise for enhancing privacy and security, particularly in regions where internet freedoms are limited.
> in certain regions, Tor and I2P connections may be filtered out
Sincere question for my understanding, are there examples of regions where clearnet is unfiltered but tor/i2p/cjdns networks are blocked?
> I now see why they were so afraid, not just of the consequences of what the country would do but response from others here.
I think you're comparing very
...
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384388036)
> it holds significant promise for enhancing privacy and security, particularly in regions where internet freedoms are limited.
> in certain regions, Tor and I2P connections may be filtered out
Sincere question for my understanding, are there examples of regions where clearnet is unfiltered but tor/i2p/cjdns networks are blocked?
> I now see why they were so afraid, not just of the consequences of what the country would do but response from others here.
I think you're comparing very
...
⚠️ cobratbq opened an issue: "External signer: in case of error shows only "External signer failure""
(https://github.com/bitcoin/bitcoin/issues/31004)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Shows a dialog with title "External signer failure" and description "External signer failure".
### Expected behaviour
Report the error that is received as part of the execution result in the `error` field designated for such execution failures/errors.
### Steps to reproduce
Have an external signer perform a `signtx` (stdin) operation and report back a result that includes a body to the
...
(https://github.com/bitcoin/bitcoin/issues/31004)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Shows a dialog with title "External signer failure" and description "External signer failure".
### Expected behaviour
Report the error that is received as part of the execution result in the `error` field designated for such execution failures/errors.
### Steps to reproduce
Have an external signer perform a `signtx` (stdin) operation and report back a result that includes a body to the
...
⚠️ cobratbq opened an issue: "Docs: "External signer" documentation is very much outdated. (plz close if unwanted request)"
(https://github.com/bitcoin/bitcoin/issues/31005)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Specification documents some basic command-line options only.
### Expected behaviour
Including missing flags, at least `--account`, `--chain`, `--std`. And in case of `--stdin`, all further stdin-commands and corresponding results.
### Steps to reproduce
N/A
### Relevant log output
N/A
### How did you obtain Bitcoin Core
Other
### What version of Bitcoin Core are you using?
maste
...
(https://github.com/bitcoin/bitcoin/issues/31005)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Specification documents some basic command-line options only.
### Expected behaviour
Including missing flags, at least `--account`, `--chain`, `--std`. And in case of `--stdin`, all further stdin-commands and corresponding results.
### Steps to reproduce
N/A
### Relevant log output
N/A
### How did you obtain Bitcoin Core
Other
### What version of Bitcoin Core are you using?
maste
...
💬 fjahr commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1781940901)
Improving the error message is reasonably easy but I didn't see a way without expanding the chain interface. I have implemented what I think is a step in the right direction but not a full solution to every edge case in order to keep the scope of this PR reasonable. Looking for feedback if people still think this is right approach and if I should expand it further or if adding more is going too far.
I think the assumeutxo + pruning use-case is pretty hard to deal with accurately for these wal
...
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1781940901)
Improving the error message is reasonably easy but I didn't see a way without expanding the chain interface. I have implemented what I think is a step in the right direction but not a full solution to every edge case in order to keep the scope of this PR reasonable. Looking for feedback if people still think this is right approach and if I should expand it further or if adding more is going too far.
I think the assumeutxo + pruning use-case is pretty hard to deal with accurately for these wal
...
💬 fjahr commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2384474829)
Improved the error messages for `importdescriptors` and `rescanblockchain` to better match the reality (usage of pruning and assumeutxo).
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2384474829)
Improved the error messages for `importdescriptors` and `rescanblockchain` to better match the reality (usage of pruning and assumeutxo).
💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2384561527)
rebased due to conflict in master
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2384561527)
rebased due to conflict in master
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781987204)
d'oh
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781987204)
d'oh
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2384565646)
reACK 2b21aebd9754c503bac12d1dbf566b3aa27451e8
via `git range-diff master 7051fcdda6fdc95677a34d5c14321d7580eceed8 2b21aebd9754c503bac12d1dbf566b3aa27451e8`
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2384565646)
reACK 2b21aebd9754c503bac12d1dbf566b3aa27451e8
via `git range-diff master 7051fcdda6fdc95677a34d5c14321d7580eceed8 2b21aebd9754c503bac12d1dbf566b3aa27451e8`
💬 petertodd commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384612509)
A good question to ask here is if Bitcoin had shipped from day zero with V2-style encryption, would we ever "upgrade" the P2P protocol to turn it off? I believe the answer is clearly no: there's no downside to encrypting the P2P layer. So with regard to long-term upgrade plans, it's fine if in the long run everything eventually moves to V2, in the same way that we've done other backwards incompatible P2P upgrades in the past. This of course is just an obscure option, so there's no rush to contem
...
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384612509)
A good question to ask here is if Bitcoin had shipped from day zero with V2-style encryption, would we ever "upgrade" the P2P protocol to turn it off? I believe the answer is clearly no: there's no downside to encrypting the P2P layer. So with regard to long-term upgrade plans, it's fine if in the long run everything eventually moves to V2, in the same way that we've done other backwards incompatible P2P upgrades in the past. This of course is just an obscure option, so there's no rush to contem
...
👍 tdb3 approved a pull request: "Mining interface: getCoinbaseMerklePath() and submitSolution()"
(https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2338978335)
Code review and light test ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
Cherry-picked `bitcoin-mine` from https://github.com/bitcoin/bitcoin/pull/30437 to exercise some of the interface.
```sh
git cherry-pick aec7e7d897741fe25fbf54995e1825772e0872d7
git cherry-pick 2227afac0372358287fb879f3b0bd07fd653f3f8
make -C depends NO_QT=1 MULTIPROCESS=1
HOST_PLATFORM="x86_64-pc-linux-gnu"
cmake -B build --toolchain=depends/$HOST_PLATFORM/toolchain.cmake
cmake --build build -j18
build/src
...
(https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2338978335)
Code review and light test ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
Cherry-picked `bitcoin-mine` from https://github.com/bitcoin/bitcoin/pull/30437 to exercise some of the interface.
```sh
git cherry-pick aec7e7d897741fe25fbf54995e1825772e0872d7
git cherry-pick 2227afac0372358287fb879f3b0bd07fd653f3f8
make -C depends NO_QT=1 MULTIPROCESS=1
HOST_PLATFORM="x86_64-pc-linux-gnu"
cmake -B build --toolchain=depends/$HOST_PLATFORM/toolchain.cmake
cmake --build build -j18
build/src
...
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782094583)
Thanks for the feedback - made the change to indicate that if `-walletdir` is specified, `loadwallet` will interpret argument relative to that directory.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782094583)
Thanks for the feedback - made the change to indicate that if `-walletdir` is specified, `loadwallet` will interpret argument relative to that directory.