💬 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
...

✅ glozow closed a pull request: "mempool: keep CPFP'd transactions when loading from mempool.dat"
(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)

💬 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

💬 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)

💬 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)

💬 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
...

💬 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.

💬 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.

💬 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`?

🚀 fanquake merged a pull request: "blockstorage: Adjust fastprune limit if block exceeds blockfile size"
(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
...

💬 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?

💬 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?

💬 ajtowns commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182176759)
I don't think locking `cs_main` or `pool.cs` for this long is okay.

💬 ajtowns commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182182757)
If `bypass_limits` is left as false, I think these changes are not necessary?

🤔 theStack reviewed a pull request: "test: added coverage to rpc_scantxoutset.py"
(https://github.com/bitcoin/bitcoin/pull/27453#pullrequestreview-1408719439)
Concept ACK,

and warm welcome as a new contributor!

Looks good to me, just one nit: the commit subject line has a missing-character-typo in the functional test filename (rpc_cantxoutset.py -> should be rpc_scantxoutset.py), can you fix that please?

💬 glozow commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182298691)
To be clear I agree that this was not the right thing to do.

💬 glozow commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1531154152)
Thanks for writing up a full review @ajtowns. Will keep this feedback in mind for the future if/when we get to this bridge again.

> implement cluster mempool first and export mempool.dat in chunks from highest to lowest fee; when importing, assume that the mempool contains chunks from highest score to lowest, build up a chunk to import; once it's over the mempool minfee, import the txs immediately; if it's over 1MvB and still under the mempool minfee, drop it and stop attempting to import txs
...

💬 glozow commented on pull request "Only allow getdata of recently announced invs":
(https://github.com/bitcoin/bitcoin/pull/19109#discussion_r1182313081)
Hope you don't mind me digging up an old PR - why do responses to mempool requests bypass the recently announced filter? And could it make sense to remove this special case? I get the idea is to give the peer access to full mempool contents, but it still seems better to only serve the stuff we announced. Concerned about `-peerbloomfilters=1` nodes getting fingerprinted through sending `mempool` + `getdata` for arbitrary transactions.