👍 TheCharlatan approved a pull request: "blockstorage: Adjust fastprune limit if block exceeds blockfile size"
(https://github.com/bitcoin/bitcoin/pull/27191#pullrequestreview-1407905862)
ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9
(https://github.com/bitcoin/bitcoin/pull/27191#pullrequestreview-1407905862)
ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9
💬 pinheadmz commented on pull request "blockstorage: Adjust fastprune limit if block exceeds blockfile size":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1181780876)
The functional test covers the `+1` nicely as well. Without the `+1` (and removing the assertion on L632) the test will hang until timed out at 30s
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1181780876)
The functional test covers the `+1` nicely as well. Without the `+1` (and removing the assertion on L632) the test will hang until timed out at 30s
👍 pinheadmz approved a pull request: "blockstorage: Adjust fastprune limit if block exceeds blockfile size"
(https://github.com/bitcoin/bitcoin/pull/27191#pullrequestreview-1407925093)
ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRQBA4ACgkQ5+KYS2KJ
yTr14BAAw1BUdgVGe3UKG5epiL1XsijS0stgQRrXVwPJKM/7O4nAy5LRQBx/ASB+
+xH0CGkd8dULO9yPrXZHXn2tjlv36Wde+ECHy76BRy6qPYgQYf/KfX0RKl7IUw3D
RNTXxYPOopXDO9h9U2jmQpwZDU7Ri3DOD5BujVq+oAu0a+782QvoTMo/Yv9mEiKr
JOyNhSzC
...
(https://github.com/bitcoin/bitcoin/pull/27191#pullrequestreview-1407925093)
ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 8f14fc86225d8fe77353f61ebd6b0bdda8d13aa9
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRQBA4ACgkQ5+KYS2KJ
yTr14BAAw1BUdgVGe3UKG5epiL1XsijS0stgQRrXVwPJKM/7O4nAy5LRQBx/ASB+
+xH0CGkd8dULO9yPrXZHXn2tjlv36Wde+ECHy76BRy6qPYgQYf/KfX0RKl7IUw3D
RNTXxYPOopXDO9h9U2jmQpwZDU7Ri3DOD5BujVq+oAu0a+782QvoTMo/Yv9mEiKr
JOyNhSzC
...
💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1181791254)
I agree with the questioning of where these util methods belong.
I didn't know about `[[nodiscard]]`. Cool feature! Added it to the public `*(to/from)Index` and private `*ToIndex` methods by this comment. New commit after update + rebase: https://github.com/bitcoin/bitcoin/pull/27534/commits/fc86267b0ece8d31915a4869741f4602622f2bcb
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1181791254)
I agree with the questioning of where these util methods belong.
I didn't know about `[[nodiscard]]`. Cool feature! Added it to the public `*(to/from)Index` and private `*ToIndex` methods by this comment. New commit after update + rebase: https://github.com/bitcoin/bitcoin/pull/27534/commits/fc86267b0ece8d31915a4869741f4602622f2bcb
💬 instagibbs commented on pull request "[POLICY] Ephemeral anchors":
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1530062410)
https://github.com/bitcoin/bitcoin/pull/26403/commits/a1982f5773690bc0c74d1bb932cb7552b00aacaf shows that current re-submission logic on node restart fails to evict ephemeral transaction.
Until there is package-aware re-submission, should these transactions just be dropped on the floor? These types of transactions are opting into fairly aggressive fee bumping patterns, maybe that's an ok first cut.
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1530062410)
https://github.com/bitcoin/bitcoin/pull/26403/commits/a1982f5773690bc0c74d1bb932cb7552b00aacaf shows that current re-submission logic on node restart fails to evict ephemeral transaction.
Until there is package-aware re-submission, should these transactions just be dropped on the floor? These types of transactions are opting into fairly aggressive fee bumping patterns, maybe that's an ok first cut.
💬 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.