💬 stickies-v commented on pull request "doc: update `BroadcastTransaction` comment":
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1468138472)
I don't understand why we'd delete it entirely either. It belongs together with the next sentence. I suppose it can be deduced from it too, but the current approach doesn't seem like an improvement to me. I still think my suggested phrasing works best from what I've seen so far. I don't think it's ambiguous, it just doesn't specify which RPC is called, which is irrelevant to the rest of the documentation and as such we shouldn't make it look like it is.

📝 brunoerg opened a pull request: "addrman: delete addresses that don't belong to the supported networks"
(https://github.com/bitcoin/bitcoin/pull/29330)
This PR adds a new flag `-cleanupaddrman`. When initializing a node with this flag, it will delete addresses from addrman (both new and tried tables) that do not belong to the supported networks (e.g. `-onlynet`). Deleting addresses that do not belong to the supported networks can avoid a lot of unnecessary addrman `Select` calls (especially when turning from clearnet + Tor/I2P/CJDNS to Tor/I2P/CJDNS). This flag is not enabled by default.

💬 stickies-v commented on pull request "Nuke adjusted time from validation (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1912691682)
> Does this mean that the adjusted time is now being collected and used exclusively to raise the warning?

This was the case before the force-push, too. In this current approach, the diff is much smaller so review can focus more on whether we're okay removing network-adjusted time, instead of on the adjusted time calculation refactoring.

💬 brunoerg commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1912698266)
cc: @mzumsande

📝 russeree opened a pull request: "redeclare nChainTx to use uint64_t"
(https://github.com/bitcoin/bitcoin/pull/29331)
Following up on https://github.com/bitcoin/bitcoin/issues/29258 the type of ```nChainTx``` has now been changed to a ```uint64_t``` allowing for the counting of TXs well into the future. The note about changing the type in 2024 was also removed. This passes all extended tests.

🤔 stickies-v reviewed a pull request: "test: Treat msg_version.relay as unsigned, Remove `struct` packing in messages.py"
(https://github.com/bitcoin/bitcoin/pull/29067#pullrequestreview-1846592629)
Concept ACK

💬 stickies-v commented on pull request "test: Treat msg_version.relay as unsigned, Remove `struct` packing in messages.py":
(https://github.com/bitcoin/bitcoin/pull/29067#discussion_r1468173825)
Aren't these supposed to be `signed=True`? Given that we're serializing a `0` I suppose it doesn't really matter there, but would still keep that consistent with the actual type.

👍 TheCharlatan approved a pull request: "depends: patch libool out of libnatpmp/miniupnpc"
(https://github.com/bitcoin/bitcoin/pull/29298#pullrequestreview-1846642861)
ACK 5b9d5bf866506b22270598aa2dc1269bc02e38e2

Guix build (x86):

```
8b32a57c3b091605070673f6338768f6ee3d5731a553ae96b426827afaf8b8f1 guix-build-5b9d5bf86650/output/aarch64-linux-gnu/SHA256SUMS.part
02a85cac626984fb8d0ea14b449af6ea916c9a3f9f9e3d68137505f5cddded60 guix-build-5b9d5bf86650/output/aarch64-linux-gnu/bitcoin-5b9d5bf86650-aarch64-linux-gnu-debug.tar.gz
f504943f0cea5bfa512a8677c8d0d1968eabcfc5a17d3fc7b810e098d7303019 guix-build-5b9d5bf86650/output/aarch64-linux-gnu/bitcoin-5b9
...

📝 russeree converted_to_draft a pull request: "redeclare nChainTx to use uint64_t"
(https://github.com/bitcoin/bitcoin/pull/29331)
Following up on https://github.com/bitcoin/bitcoin/issues/29258 the type of ```nChainTx``` has now been changed to a ```uint64_t``` allowing for the counting of TXs well into the future. The note about changing the type in 2024 was also removed. This passes all extended tests.

💬 achow101 commented on pull request "test: ensure output is large enough to pay for its fees":
(https://github.com/bitcoin/bitcoin/pull/29283#issuecomment-1912825257)
ACK 3bfc5bd36e38ca07306a41a3f56ea102ffe02b67

🚀 achow101 merged a pull request: "test: ensure output is large enough to pay for its fees"
(https://github.com/bitcoin/bitcoin/pull/29283)

💬 achow101 commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1912838551)
Could you clarify what deprecation will actually mean for 27.0, and then the future steps?

From what I understand. this PR removes a bunch of tests, and adds a sentence to a doc saying that it is deprecated. I think that means we will still be building the library by default but not testing it? That seems a bit odd to me, and I would rather that testing is deleted at the same time the library is removed, unless there is some reason that those tests can be removed that is unrelated to the depr
...

🚀 achow101 merged a pull request: "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256"
(https://github.com/bitcoin/bitcoin/pull/29180)

💬 theStack commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#discussion_r1468264402)
> FillableSigningProvider is just the legacy class and contains legacy scripts limitations (thus https://github.com/bitcoin/bitcoin/pull/28307). It does not affect the current form of this PR but, because this benchmark uses segwit v0 and v1 outputs, it could cause issues in the future.

Ok, good to know.

> I never said that it was going to make derivation easier. I Just said that it will also save you an extra GetScriptForDestination call per script.

Took me a while now to understand wh
...

💬 shaavan commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1913035371)
Thanks, @vostrnad, for pointing out the potential flaws in my review.

Yes, I did use ChatGPT to structure my thoughts on the “problems with P2PK” in a coherent fashion, but I don’t believe the points are wrong.

Taking references from, Programming Bitcoin, Jimmy Song

https://github.com/jimmysong/programmingbitcoin/blob/master/ch06.asciidoc#problems-with-p2pk

1. Problem of Security: P2PK, exposes the bare public key to the world, and if ever ECDSA gets broken, we might lose our funds

...

💬 vostrnad commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1913056465)
@shaavan

1. Putting bare public keys into the output doesn't make things materially worse if ECDSA/ECDLP gets broken. This was agreed on when designing Taproot, which is why P2TR outputs also use bare public keys (see [BIP341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_note-2)).
2. Yes, P2PKH has a shorter output script, but overall uses more blockspace than P2PK. It does take less space in the UTXO set, but P2PK only takes one more byte than e.g. P2TR or P2WSH. Note
...

💬 shaavan commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1913088852)
Thanks, @vostrnad, for helping me bring clarity to my understanding.

I understand that my points were made based on my limited knowledge surrounding the discussions around P2TR. And also, I made an incorrect point in my explanation.

I have struck the “Problems of P2PK” section to avoid confusion for future reviewers.

Lastly, though I might sometimes use ChatGPT to get better clarity on concepts or coherently express my thoughts, I will make sure to properly fact-check it before using an
...

💬 ArmchairCryptologist commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1913096657)
> Hardcoded one-size-fits-all configuration is suboptimal, especially in context of mempools which are always unique (by definition).

If you do not run a mining node, you do in fact want your mempool configuration to be as compatible with the miner majority as possible if you want it to be "optimal", both from the perspective of your node and from the network as a whole. In most cases, this means using the defaults, but possibly with a larger size.

To see why, let's assume that you configu
...

💬 ajtowns commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1913161408)
> Could you clarify what deprecation will actually mean for 27.0, and then the future steps?

I would have thought that for 27.x you'd just add the deprecation note, and otherwise not change any code? I assumed the code changes here were just to help make commenting easier, not necessarily intended for 27.0...

> For 27.0 specifically, I think this needs a release note. Release notes at least have some expectation of being read regularly, so a deprecation notice in there would be beneficial
...

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1468498323)
@mzumsande, @sr-gi, I played with @sr-gi's idea on [this branch](https://github.com/stratospher/bitcoin/commit/276b95d766cd11d65d563805d52beace8dd32744) and wondering about 2 things:

1. can deadlocks happen? i noticed places in the test where `_send_lock` was acquired and then `p2p_lock` (and deadlocks didn't happen). am i missing something?
1. MainThread acquires `_send_lock`
2. NetworkThread acquires `p2p_lock`
3. NetworkThread would wait for MainThread to release `_send_lock`
4. Ma
...