💬 LaurentMT commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3070834011)
DUST_RELAY_TX_FEE should be decreased along with this as well...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3070834011)
DUST_RELAY_TX_FEE should be decreased along with this as well...
💬 Sjors commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3070838431)
Another thing I noticed while testing using the `send` RPC, in a wallet with _only_ an `sp()` descriptor, is that it insists on having a bech32 descriptor for change:
> Transaction needs a change address, but we can't generate it. Error: No bech32 addresses available.
It seems safer, for backups, to send change to the same silent payment address.
Wallet created by:
```sh
bitcoin rpc createwallet Silent blank=true silent_payments=true
bitcoin rpc importdescriptors '[{"desc": "sp(xpr
...
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3070838431)
Another thing I noticed while testing using the `send` RPC, in a wallet with _only_ an `sp()` descriptor, is that it insists on having a bech32 descriptor for change:
> Transaction needs a change address, but we can't generate it. Error: No bech32 addresses available.
It seems safer, for backups, to send change to the same silent payment address.
Wallet created by:
```sh
bitcoin rpc createwallet Silent blank=true silent_payments=true
bitcoin rpc importdescriptors '[{"desc": "sp(xpr
...
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691366)
Right, moved
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691366)
Right, moved
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691859)
oh weird! added
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691859)
oh weird! added
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205723195)
Thanks! Added, I should have just done this.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205723195)
Thanks! Added, I should have just done this.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205692104)
done, thanks
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205692104)
done, thanks
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205723116)
left it in for now.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205723116)
left it in for now.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691617)
added
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691617)
added
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205722748)
added
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205722748)
added
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205719816)
CAUGHT 🔫
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205719816)
CAUGHT 🔫
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691462)
added
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205691462)
added
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205735550)
Yeah it was originally implemented that way, but I think this way likely requires much fewer element lookups. Under "normal" conditions, we probably have multiple announcers for each transaction and there are very children from this peer.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205735550)
Yeah it was originally implemented that way, but I think this way likely requires much fewer element lookups. Under "normal" conditions, we probably have multiple announcers for each transaction and there are very children from this peer.
🤔 mzumsande reviewed a pull request: "rpc,net: Add getpeerbyid and listpeersbyids RPCs"
(https://github.com/bitcoin/bitcoin/pull/32972#pullrequestreview-3017872764)
> yes, the option to retrieve result by peer id.
Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
(https://github.com/bitcoin/bitcoin/pull/32972#pullrequestreview-3017872764)
> yes, the option to retrieve result by peer id.
Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
💬 waketraindev commented on pull request "rpc,net: Add getpeerbyid and listpeersbyids RPCs":
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3071047468)
> > yes, the option to retrieve result by peer id.
>
> Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
> > yes, the option to retrieve result by peer id.
>
> Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
Yes, this is essentially a more complete version of that, incorporating feedback and suggestions from other contributors. It serves as an alternative approach to PR #32741.
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3071047468)
> > yes, the option to retrieve result by peer id.
>
> Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
> > yes, the option to retrieve result by peer id.
>
> Isn't this what your other PR #32741 suggests? Is this meant to be an alternative to that?
Yes, this is essentially a more complete version of that, incorporating feedback and suggestions from other contributors. It serves as an alternative approach to PR #32741.
🤔 furszy reviewed a pull request: "index: initial sync speedup, parallelize process"
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-3017897441)
> Sorry, #32948 probably caused some conflicts.
Auch. Rebased.
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-3017897441)
> Sorry, #32948 probably caused some conflicts.
Auch. Rebased.
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3071149725)
> It seems that since m_block_index is a std::unordered_map, the iteration order in RecalculateBestHeader is not guaranteed.
Oh yes, this bit me in an earlier iteration of this PR where I made the mistake to do `PickValue(fuzzed_data_provider, blockman.m_block_index)` directly to pick a random block index (so that no `blocks` vector was needed).
It's also related to the fact that we don't use `CBlockIndexWorkComparator` tiebreak rules for `m_best_header` (documented in #32843), so I think
...
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3071149725)
> It seems that since m_block_index is a std::unordered_map, the iteration order in RecalculateBestHeader is not guaranteed.
Oh yes, this bit me in an earlier iteration of this PR where I made the mistake to do `PickValue(fuzzed_data_provider, blockman.m_block_index)` directly to pick a random block index (so that no `blocks` vector was needed).
It's also related to the fact that we don't use `CBlockIndexWorkComparator` tiebreak rules for `m_best_header` (documented in #32843), so I think
...
👋 waketraindev's pull request is ready for review: "rpc,net: Add getpeerbyid and listpeersbyids RPCs"
(https://github.com/bitcoin/bitcoin/pull/32972)
(https://github.com/bitcoin/bitcoin/pull/32972)
💬 tnndbtc commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3071393448)
I assume the general code logic should be correct, but likely some exception/run-time error occurred then caused the intermittent issue.
So, I studied `Popen::execute_process()` in src/util/subprocess.h, one interesting statement caught my attention:
```
subprocess_close(err_wr_pipe);// close child side of pipe, else get stuck in read below
stream_.close_child_fds();
```
I notice that we didn't check the return code of the calling of `subprocess_close()/close()`, in many cases. Indeed,
...
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3071393448)
I assume the general code logic should be correct, but likely some exception/run-time error occurred then caused the intermittent issue.
So, I studied `Popen::execute_process()` in src/util/subprocess.h, one interesting statement caught my attention:
```
subprocess_close(err_wr_pipe);// close child side of pipe, else get stuck in read below
stream_.close_child_fds();
```
I notice that we didn't check the return code of the calling of `subprocess_close()/close()`, in many cases. Indeed,
...
💬 andrewtoth commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3071549529)
> DUST_RELAY_TX_FEE should be decreased along with this as well...
That value is used to calculate the minimum output amount, not the fee rate of a tx. That seems orthogonal to the goal of this PR.
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3071549529)
> DUST_RELAY_TX_FEE should be decreased along with this as well...
That value is used to calculate the minimum output amount, not the fee rate of a tx. That seems orthogonal to the goal of this PR.
💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2206105512)
Ok, you are right on the `migratewallet` process, I've missed that, perhaps we can't go around that one (`CanSupportFeature(FEATURE_HD_SPLIT)`). The one in `getwalletinfo`, the condition itself can be removed, leaving the push of `keypoolsize_hd_internal`, as since legacy wallets can't no longer be loaded, can't run this command on them. But I doubt this is part of the scope of this PR, maybe can be done in a follow-up, like removing the rest of the functions (`GetVersion()`, `GetClosestWalletFe
...
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2206105512)
Ok, you are right on the `migratewallet` process, I've missed that, perhaps we can't go around that one (`CanSupportFeature(FEATURE_HD_SPLIT)`). The one in `getwalletinfo`, the condition itself can be removed, leaving the push of `keypoolsize_hd_internal`, as since legacy wallets can't no longer be loaded, can't run this command on them. But I doubt this is part of the scope of this PR, maybe can be done in a follow-up, like removing the rest of the functions (`GetVersion()`, `GetClosestWalletFe
...