💬 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.
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782094771)
Sounds fair to me - made this change 👍
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782094771)
Sounds fair to me - made this change 👍
💬 maflcko commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782120451)
This is a breaking change and will cause the tests to fail, not a documentation change
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782120451)
This is a breaking change and will cause the tests to fail, not a documentation change
💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2384840093)
> the next step should be revert #26564?.
IIUC the motivation for 26564 was to have a fully static and fixed path for the unit test datadir. My understanding is that changing the temp storage device was just a side-effect and not the primary motivation. (My comment was just a note to say that your goal is already achievable today)
> It seems more natural to plug the already existing -testdatadir arg to the benchmark framework.
I agree. I've also suggested this in https://github.com/bitc
...
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2384840093)
> the next step should be revert #26564?.
IIUC the motivation for 26564 was to have a fully static and fixed path for the unit test datadir. My understanding is that changing the temp storage device was just a side-effect and not the primary motivation. (My comment was just a note to say that your goal is already achievable today)
> It seems more natural to plug the already existing -testdatadir arg to the benchmark framework.
I agree. I've also suggested this in https://github.com/bitc
...
💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1782146967)
```suggestion
| [Qt](../depends/packages/qt.mk) | [link](https://download.qt.io/official_releases/qt/) | [6.7.2](https://github.com/bitcoin/bitcoin/pull/30997) | [6.2](https://github.com/bitcoin/bitcoin/pull/30997) | No |
```
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1782146967)
```suggestion
| [Qt](../depends/packages/qt.mk) | [link](https://download.qt.io/official_releases/qt/) | [6.7.2](https://github.com/bitcoin/bitcoin/pull/30997) | [6.2](https://github.com/bitcoin/bitcoin/pull/30997) | No |
```
💬 Mackain commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2384959633)
> ACK [3208df2](https://github.com/bitcoin/bitcoin/commit/3208df2a100f58569165081cd20c02abed827286)
>
> Might be good to update the PR description to reflect the changes (and the commit message as well, if you need to repush).
>
> Suggestion for the PR title: `doc: update IBD requirements in doc/README.md`
@jonatack thanks! Updated the PR title as you suggested. The commit message was created automatically when I clicked the "Commit Suggestion" button on your previous feedback. Fixing i
...
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2384959633)
> ACK [3208df2](https://github.com/bitcoin/bitcoin/commit/3208df2a100f58569165081cd20c02abed827286)
>
> Might be good to update the PR description to reflect the changes (and the commit message as well, if you need to repush).
>
> Suggestion for the PR title: `doc: update IBD requirements in doc/README.md`
@jonatack thanks! Updated the PR title as you suggested. The commit message was created automatically when I clicked the "Commit Suggestion" button on your previous feedback. Fixing i
...