💬 conduition commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1853421238)
> What fundamentally bothers me with inscriptions is that they do not play the same game as those who make financial transactions.

Thanks for that explanation @Retropex. I like [the chess club analogy linked in that thread](https://nitter.net/Conza/status/1622049397801635840#m), it feels very appropriate.

So - not to generalize this to everyone supporting this PR - the driving motivation here isn't to prevent _all_ spam, but to prevent a particular _flavor_ of transaction, which people hav
...

💬 conduition commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1853422737)

I'd like to demonstrate why it is wildly impractical to even _detect_ arbitrary data push transactions, let alone filter them reliably.

A subtle workaround to this PR:

1. Encrypt the push-data/inscription
2. Format the ciphertext as hashes, pubkeys, and op-code sequences.
3. Encode those hashes, pubkeys, etc into a valid-looking script pubkey.
4. Use that script pubkey as the inscription envelope.
5. Publish the decryption key _after_ the ciphertext is already included in a block.
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1424980831)
Will fix if i end up retouching it :)

💬 nikodemas commented on issue "test: Add TestNode wait_until helper":
(https://github.com/bitcoin/bitcoin/issues/29029#issuecomment-1853440622)
Hi @mohamedawnallah , still working :)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1853445405)
@mzumsande Previously I also felt it's unfortunate, but never found a good way to fix it. Now I think perhaps we should compute `GetFanoutTargets` once for every transaction, and then memorize it in a `std::map<Wtxid, list_of_peers>` inside the TxReconciliation module. We can cap the map at 1000 in a FIFO-fashion. Worst case we drop a relevant transaction and then have to recompute it (that's current behaviour default case). Memory should be ok too: `1000 txs * 120 peers * 0.1 ratio = 12,000 ent
...

💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1853452284)
@luke-jr

> Can you move the off-topic chatter somewhere else? This PR is strictly a bugfix for existing functionality. Whether you agree or disagree with that functionality is irrelevant.

There is not consensus that this PR is a bugfix. Thus all this discussion is relevant.

> P.S. [OP_RETURN was originally for sending coins to miners](https://buildingbitcoin.org/bitcoin-dev/log-2012-08-20.html#L-1512)

You are refering to speculative discussion. As you know, OP_Return's functionality
...

💬 naumenkogs commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1424997744)
To be clear, i suggested literally having `Assume(fDisconnect)`, not an extra `if` check (that would actually make code worse).

Even if you refactor it into `ForEachFullyConnectedNode`, I think this `Assume(fDisconnect)` is kinda useful — because who knows what exactly `ForEachFullyConnectedNode` means :)

But I'm fine either way, either you rename or add `Assume` (both are ideal i think :))

💬 ajtowns commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1425022656)
> With this background, I thought for this PR putting `TransactionError` in `node` instead of `common` would be better so it would be more likely to be appropriately used for functions like `node::BroadcastTransaction` and `interfaces::Node::broadTransaction`, and less likely to be misused for non-node functions. I could understand the opposite argument, though, that if even if it belongs in `node` when used ideally, it makes more sense for it to be in `common` based on the way it is used now.

...

💬 naumenkogs commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1853485863)
@theStack I think the philosophy behind v2 was always "prevent the firewall from discriminating authenticated connections by making them look exactly like normal connections". From [BIP](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki):

>Encryption, even when it is unauthenticated and only used when both endpoints support v2, impedes eavesdropping by forcing the attacker to become active: either by performing a persistent man-in-the-middle (MitM) attack, by downgrading connect
...

💬 naumenkogs commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1425044177)
c5d41370287f48c638ab87d3e02a117b1a17694a

What if the peer is `v2`, but disconnected us due to other reason? (say a limit of inbounds, or us coming from a blocked network)? Here and in the logic below. In that case we should not attempt reconnection with `v1`, it won't help.

This is probably about `ShouldReconnectV1` behavior. I can't find where this case is handled.

💬 dergoegge commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1853532671)
This is still broken, please reopen:
```
$ echo "AQ0N0NfM0B7QEK1AL3m3AJWVlQAAgQAAAAAAlZWVAACBAAAAAACVlZUAAAAAAAAAAAAAAAAAXQAAgQAAAAAAAAAAAAAA" | base64 --decode > coinselection-90005214eeb91a419ca37def8d958bf6bb670edf.crash
$ FUZZ=coinselection ./src/test/fuzz/fuzz coinselection-90005214eeb91a419ca37def8d958bf6bb670edf.crash
fuzz_libfuzzer: wallet/test/fuzz/coinselection.cpp:122: void wallet::coinselection_fuzz_target(FuzzBufferType): Assertion `result_bnb->GetChange(coin_params.m_cost_of_ch
...

⚠️ maflcko reopened an issue: "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed"
(https://github.com/bitcoin/bitcoin/issues/28918)
```
$ echo "PACVlVuVlZWVlZUE3pUAANNRAFEA09NRUb9RUVFR/wD/AP//AP8AWwD//wcErq6urv///////wD/4wAAAAD4a9cA////ra2tra2tra2tra2VlZWVMGA5OTk5OZWVlZWVlZWVlZWVlZWVlYUVJwyq6wEAlZWblZWVlZWVlZWVlZWVlZWblZWVlZWVlZWVlZWV//+V/5WV/////z4AAAAALAtfAgAAX/8=" | base64 --decode > coinselection-d97eed2ff63da56af72c8c858c560a7c6f2aef45.crash
$ FUZZ=coinselection ./src/test/fuzz/fuzz coinselection-d97eed2ff63da56af72c8c858c560a7c6f2aef45.crash
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 189972
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1853542223)
It's not easy being green, but including `compat.h` should fix the issue:

```cpp
// ssize_t is POSIX, and not present when using MSVC.
```

The CI fuzzer caught an overflow, so I added a guard against absurd `coinbase_output_max_additional_size` values. I have not tried the fuzzer myself yet, nor does it have complete coverage, so there may still be other issues.

💬 fanquake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1853546307)
See https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1853532671. This doesn't seem to have fixed the fuzzer.

💬 paulocoutinhox commented on issue "CMake-based build system tracking issue":
(https://github.com/bitcoin/bitcoin/issues/28607#issuecomment-1853551871)
+1

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1425077125)
Ah, you're right, that'd be bad.

💬 fanquake commented on issue "[bench] Assertion error on wallet_create_tx.cpp, line 133.":
(https://github.com/bitcoin/bitcoin/issues/29069#issuecomment-1853562050)
Duplicate of #29061.

✅ fanquake closed an issue: "[bench] Assertion error on wallet_create_tx.cpp, line 133."
(https://github.com/bitcoin/bitcoin/issues/29069)

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425084623)
Ok the lazy way clearly doesn't work; I think what we have to do is calculate in-package ancestors and feed that to `ApplyV3Rules` when we call it. This will also have the effect of consolidating our v3 checks so that it's cleaner. Pushing soon...

💬 fanquake commented on pull request "ci: Use Ubuntu 24.04 Noble for asan,tsan,tidy,fuzz":
(https://github.com/bitcoin/bitcoin/pull/28992#issuecomment-1853568692)
> Will be fixed in 17.0.6-2 once noble-proposed is released,

This has shipped, and `17.0.6-2` is available.