💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1424611283)
Doesn't this leave an unsafe dangling reference if we don't have a variable capturing ownership of the `unique_ptr`?

💬 mzumsande 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_r1424611995)
I like that name! I missed your comment in the most recent push, but will change it tomorrow!

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1424659497)
This defeats the purpose of this pull because it generates a randomly named data directory path; the point of this PR is to have a fixed datadir pathname so that I can have various files open in an editor across multiple test runs (not have to determine files' locations and re-open them on each test run).

> Be forced to place the test only inside the temp directory doesn't seems so useful to me.

Well, if we want the datadir pathnames to be static (which is what _I'm_ trying to accomplish w
...

💬 luke-jr commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1853037139)
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.

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

👍 theStack approved a pull request: "test: Actually fail when a python unit test fails"
(https://github.com/bitcoin/bitcoin/pull/29068#pullrequestreview-1778639038)
ACK fa0534d7e47d44428d3f9dea6d2f6b8e86df22d4

⚠️ kevkevinpal opened an issue: "[bench] Assertion error on wallet_create_tx.cpp, line 133."
(https://github.com/bitcoin/bitcoin/issues/29069)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

On bitcoin/master d646ca35d991e4f694096fdbd2d2ebd8cebf244e
I compiled bitcoin core with bench on macOS and tried to run the benchmarks but got an error as such
`Assertion failed: (tx_res), function operator(), file wallet_create_tx.cpp, line 133.`

this is the last few logs I see before it fails
```
./src/bench/bench_bitcoin

| ns/op | op/s | err% |
...

💬 kevkevinpal commented on issue "[bench] Assertion error on wallet_create_tx.cpp, line 133.":
(https://github.com/bitcoin/bitcoin/issues/29069#issuecomment-1853128008)
Seems like this one specifically is giving my machine trouble
`./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection`

💬 furszy commented on issue "[bench] Assertion error on wallet_create_tx.cpp, line 133.":
(https://github.com/bitcoin/bitcoin/issues/29069#issuecomment-1853153904)
See #29065.

💬 kevkevinpal commented on issue "[bench] Assertion error on wallet_create_tx.cpp, line 133.":
(https://github.com/bitcoin/bitcoin/issues/29069#issuecomment-1853169756)
> See #29065.

thank you will pull and review the PR!

💬 kevkevinpal commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1853172985)
ACK [37c75c5](https://github.com/bitcoin/bitcoin/pull/29065/commits/37c75c58202f89b752500f76c872d7f8caf6bdb3)

closes https://github.com/bitcoin/bitcoin/issues/29069

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1853174830)
Force pushed suggestion by @furszy (thanks!) https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1424023270

💬 0xakihiko commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1853335872)
> 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.
>
> P.S. [OP_RETURN was originally for sending coins to miners](https://buildingbitcoin.org/bitcoin-dev/log-2012-08-20.html#L-1512)

Because that's how we fix bugs. One line commits, broken test cases, broken ecosystems and a totalitarian view of discussions.

👍 BrandonOdiwuor approved a pull request: "test: Actually fail when a python unit test fails"
(https://github.com/bitcoin/bitcoin/pull/29068#pullrequestreview-1778932595)
ACK fa0534d7e47d44428d3f9dea6d2f6b8e86df22d4

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