💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#issuecomment-1530063074)
Thank you for taking a look @jonatack! :pray: All your comments on code organization are fair game and something I've wondered myself. Because I have absolutely zero point of reference for any of this, I'm going to to leave your comments about organization open with the hopes of getting more feedback.
(https://github.com/bitcoin/bitcoin/pull/27534#issuecomment-1530063074)
Thank you for taking a look @jonatack! :pray: All your comments on code organization are fair game and something I've wondered myself. Because I have absolutely zero point of reference for any of this, I'm going to to leave your comments about organization open with the hopes of getting more feedback.
🤔 instagibbs reviewed a pull request: "mempool: keep CPFP'd transactions when loading from mempool.dat"
(https://github.com/bitcoin/bitcoin/pull/27476#pullrequestreview-1407953553)
approach ACK
(https://github.com/bitcoin/bitcoin/pull/27476#pullrequestreview-1407953553)
approach ACK
💬 instagibbs commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1181800707)
`removed` is a bit confusing since it will likely bail without doing anything. `to_remove_rate`?
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1181800707)
`removed` is a bit confusing since it will likely bail without doing anything. `to_remove_rate`?
💬 instagibbs commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1181824474)
To recap, this disallows a wallet(or other TrimToSize caller) from trimming a parent before the child is entered into the mempool?
With debug builds, this draws out the startup time of bitcoind by ~1.5 minutes with 300MB mempool. ~30 seconds on non-debug build.
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1181824474)
To recap, this disallows a wallet(or other TrimToSize caller) from trimming a parent before the child is entered into the mempool?
With debug builds, this draws out the startup time of bitcoind by ~1.5 minutes with 300MB mempool. ~30 seconds on non-debug build.
💬 fjahr commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1530111029)
I finally got another amd64 machine with which I can test again. I confirmed the clang-14 results others have seen and I tested with GCC 13.1. The results still look good there for me but would be great if someone else could confirm.
(using `src/bench/bench_bitcoin -filter=MuHash.* -min-time=1000`)
Master:
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 9,000.29
...
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1530111029)
I finally got another amd64 machine with which I can test again. I confirmed the clang-14 results others have seen and I tested with GCC 13.1. The results still look good there for me but would be great if someone else could confirm.
(using `src/bench/bench_bitcoin -filter=MuHash.* -min-time=1000`)
Master:
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 9,000.29
...
💬 michaelfolkson commented on issue "rpc: show P2(W)SH redeemScript in getrawtransaction (and friends)":
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-1530116738)
@Riahiamirreza: I suspect IRC (#bitcoin-core-pr-reviews) is better for these conversations as some have email notifications turned on for every comment in this repo. I wouldn't have thought there was a backward compatibility issue here if you are adding a field rather than say removing a field that external software is relying upon.
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-1530116738)
@Riahiamirreza: I suspect IRC (#bitcoin-core-pr-reviews) is better for these conversations as some have email notifications turned on for every comment in this repo. I wouldn't have thought there was a backward compatibility issue here if you are adding a field rather than say removing a field that external software is relying upon.
🤔 glozow reviewed a pull request: "wallet: tx creation, don't select outputs from txes that are being replaced"
(https://github.com/bitcoin/bitcoin/pull/26732#pullrequestreview-1408042454)
Concept ACK, apologies for the late re-review
Just to recap the whiteboarding session we had offline, I think what we want is to add a `SanitizeCoinControl` function that can be used early on across both bumpfee and fundraw RPCs:
1. If any of the selected inputs is already spent in mempool, `is_replacement=true` and keep track of which inputs are being spent again.
2. If the selected inputs includes UTXOs spent and created by the same transaction (i.e. selected output from a tx that is be
...
(https://github.com/bitcoin/bitcoin/pull/26732#pullrequestreview-1408042454)
Concept ACK, apologies for the late re-review
Just to recap the whiteboarding session we had offline, I think what we want is to add a `SanitizeCoinControl` function that can be used early on across both bumpfee and fundraw RPCs:
1. If any of the selected inputs is already spent in mempool, `is_replacement=true` and keep track of which inputs are being spent again.
2. If the selected inputs includes UTXOs spent and created by the same transaction (i.e. selected output from a tx that is be
...
💬 glozow commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1530227211)
Closing this for now to (1) focus on more important functionality (2) defer the decision-making to when we have more information on usage and/or better eviction.
- The "trim stuff below minrelayfeerate" thing isn't very good... as established, we might evict things we shouldn't, etc.
- This isn't strictly necessary yet until package relay is deployed.
- It might be too early to decide on a solution. Perhaps with EA and actual package relay usage, we'll end up wanting to reformat mempool.dat a
...
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1530227211)
Closing this for now to (1) focus on more important functionality (2) defer the decision-making to when we have more information on usage and/or better eviction.
- The "trim stuff below minrelayfeerate" thing isn't very good... as established, we might evict things we shouldn't, etc.
- This isn't strictly necessary yet until package relay is deployed.
- It might be too early to decide on a solution. Perhaps with EA and actual package relay usage, we'll end up wanting to reformat mempool.dat a
...
✅ glozow closed a pull request: "mempool: keep CPFP'd transactions when loading from mempool.dat"
(https://github.com/bitcoin/bitcoin/pull/27476)
(https://github.com/bitcoin/bitcoin/pull/27476)
👋 glozow's pull request is ready for review: "validate package transactions with their in-package ancestor sets"
(https://github.com/bitcoin/bitcoin/pull/26711)
(https://github.com/bitcoin/bitcoin/pull/26711)
💬 brunoerg commented on pull request "test: merge banning test from p2p_disconnect_ban to rpc_setban":
(https://github.com/bitcoin/bitcoin/pull/26863#issuecomment-1530448395)
Rebased
(https://github.com/bitcoin/bitcoin/pull/26863#issuecomment-1530448395)
Rebased
💬 Meru852 commented on issue "rpc: show P2(W)SH redeemScript in getrawtransaction (and friends)":
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-1530708848)
some one can help me to solve this script. cause i can't get the true balance of the tx.
from bitcoinaddress import *
while True:
wallet=Wallet()
print(wallet)
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-1530708848)
some one can help me to solve this script. cause i can't get the true balance of the tx.
from bitcoinaddress import *
while True:
wallet=Wallet()
print(wallet)
💬 Meru852 commented on issue "Export a watch wallet only (with descriptors and without private keys) for an air gap setup":
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1530723790)
some one please help me to finish this script i made cause i can't get the true balance of running tx.
from bitcoinaddress import *
while True:
wallet=Wallet()
print(wallet)
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1530723790)
some one please help me to finish this script i made cause i can't get the true balance of running tx.
from bitcoinaddress import *
while True:
wallet=Wallet()
print(wallet)
💬 willcl-ark commented on issue "build: configure using depends by default if config.site exists ":
(https://github.com/bitcoin/bitcoin/issues/16692#issuecomment-1531042533)
@fanquake @hebasto mentions [here](https://github.com/hebasto/bitcoin/pull/10#issuecomment-1530100470) that:
> I no longer consider this feature as a useful one due to the case, which is actually an everyday workflow for me, when the user has depends built for many hosts simultaneously.
Intuitively I would estimate the number of users building depends for multiple hosts to be significantly less than the number building for their own host (especially since we have https://github.com/bitcoin
...
(https://github.com/bitcoin/bitcoin/issues/16692#issuecomment-1531042533)
@fanquake @hebasto mentions [here](https://github.com/hebasto/bitcoin/pull/10#issuecomment-1530100470) that:
> I no longer consider this feature as a useful one due to the case, which is actually an everyday workflow for me, when the user has depends built for many hosts simultaneously.
Intuitively I would estimate the number of users building depends for multiple hosts to be significantly less than the number building for their own host (especially since we have https://github.com/bitcoin
...
💬 real-or-random commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1531069839)
> `#if (defined(__amd64__) || defined(__x86_64__)) && defined(__GNUC__) && !defined(__clang__)`
If it's that niche, it's a bit unclear to me whether it's worth the hassle. I feel we should look at #21590 first. I expect this to be a much larger improvement (and since it's algorithmic, it will apply to all targets). Perhaps we don't care about this optimization here so much after #21590.
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1531069839)
> `#if (defined(__amd64__) || defined(__x86_64__)) && defined(__GNUC__) && !defined(__clang__)`
If it's that niche, it's a bit unclear to me whether it's worth the hassle. I feel we should look at #21590 first. I expect this to be a much larger improvement (and since it's algorithmic, it will apply to all targets). Perhaps we don't care about this optimization here so much after #21590.
💬 Tracachang commented on issue "Write instructions on offline signing.":
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1531089137)
>
Thank you, just let me know how to proceed in case you want me to rewrite the tutorial or need me to add more information. Always happy to help.
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1531089137)
>
Thank you, just let me know how to proceed in case you want me to rewrite the tutorial or need me to add more information. Always happy to help.
💬 MarcoFalke commented on pull request "fuzz: addrman, add coverage for `network` field in `Select()`":
(https://github.com/bitcoin/bitcoin/pull/27549#discussion_r1182254444)
Any reason to not use `ALL_NETWORKS`?
(https://github.com/bitcoin/bitcoin/pull/27549#discussion_r1182254444)
Any reason to not use `ALL_NETWORKS`?
🚀 fanquake merged a pull request: "blockstorage: Adjust fastprune limit if block exceeds blockfile size"
(https://github.com/bitcoin/bitcoin/pull/27191)
(https://github.com/bitcoin/bitcoin/pull/27191)
🤔 ajtowns reviewed a pull request: "mempool: keep CPFP'd transactions when loading from mempool.dat"
(https://github.com/bitcoin/bitcoin/pull/27476#pullrequestreview-1408532061)
Approach NACK: I don't think holding cs_main or the mempool lock for the full import period is okay.
I'm not sure this needs fixing at all at this point: it doesn't affect nodes that don't restart in between the tx being broadcast and mined; it doesn't affect nodes that don't have a full mempool (eg, if you raise the limit to 1GB or 2GB instead of 300MB); and it's not needed if the child tx is either rebroadcast or rbf'ed. In some sense, saving the mempool and reloading it on restart is just
...
(https://github.com/bitcoin/bitcoin/pull/27476#pullrequestreview-1408532061)
Approach NACK: I don't think holding cs_main or the mempool lock for the full import period is okay.
I'm not sure this needs fixing at all at this point: it doesn't affect nodes that don't restart in between the tx being broadcast and mined; it doesn't affect nodes that don't have a full mempool (eg, if you raise the limit to 1GB or 2GB instead of 300MB); and it's not needed if the child tx is either rebroadcast or rbf'ed. In some sense, saving the mempool and reloading it on restart is just
...
💬 ajtowns commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182180279)
We currently only do `bypass_limits` for txs from a reorg; not even packages can bypass limits otherwise. I think leaving `bypass_limits=false` for loading from mempool.dat should be fine until that changes?
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182180279)
We currently only do `bypass_limits` for txs from a reorg; not even packages can bypass limits otherwise. I think leaving `bypass_limits=false` for loading from mempool.dat should be fine until that changes?
💬 ajtowns commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182181426)
This may be evicting txs that were present in the mempool prior to the import, in which case it ill overestimate failed and underestimate count?
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182181426)
This may be evicting txs that were present in the mempool prior to the import, in which case it ill overestimate failed and underestimate count?