💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1840346467)
Thanks!
Applied following some of the examples.

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840354778)
Correct, this is an oversight from an older version of this PR. Likely related to https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1692884230.

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1840359897)
Left a comment, I think my one-liner is the cleanest correct solution?

pushed it as a separate commit

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840396964)
But we are adding a new option here `privatebroadcast`. Could we not change the semantics of `connect` *if* the new option `privatebroadcast` is used?

👍 instagibbs approved a pull request: "cluster mempool: Implement changeset interface for mempool"
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2433424943)
ACK 0ca1e05d8bcbc42892156c15f128a5f829e9e48e

primary change is the tracing commit, which seems to make sense though I am not an expert

via `git range-diff master 9512bd34a3d74d15c3ce1086a80a1edfa5ac3acf 0ca1e05d8bcbc42892156c15f128a5f829e9e48e`

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840447218)
Good catch. I confirmed that the `a`'s actually show up in the tracing scripts and are cut off. I think it's fine to leave 68 in the tests, but I'll add a footnote in the docs in a separate commit.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840474930)
Yeah, you are right that `-connect=0 -privatebroadcast=1` did not exist before because `-privatebroadcast=` did not exist. Then the semantic of `-connect=0` would be "do this if privatebroadcast=0 and do something else if privatebroadcast=1". I do not like that, seems convoluted and a possible source of confusion to make the behavior of one option dependent on the value of another option. I would like to have less of that stuff, not more.

💬 fanquake commented on issue "TSAN/MSAN fails with vm.mmap_rnd_bits=32 even with llvm 18.1.3":
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2473835423)
Has started happening again in the TSAN CI (Clang 19.1.4)? https://cirrus-ci.com/task/5338932714405888?logs=ci#L2213

💬 instagibbs commented on pull request "validation: Remove RECENT_CONSENSUS_CHANGE validation result":
(https://github.com/bitcoin/bitcoin/pull/31269#issuecomment-2473840395)
@sdaftuar since you're the one who introduced this flag IIRC, mind weighing in?

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840483719)
I think this can be handled on the user side. The bpftrace tool cuts of strings after 64 chars by default and bcc and libbpf both have [bpf_probe_read_user_str()](https://docs.ebpf.io/linux/helper-function/bpf_probe_read_user_str/), which would replace the last char with `\0` on overflow.

I haven't had any problems with this using the tracepoints in Python and Rust. I did a quick check with a C program, and using `bpf_probe_read_user_str()` seems fine: https://github.com/0xB10C/libbpf-bootst
...

💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2473845095)
@fjahr @ismaelsadeeq ``txfeerate`` is now deprecated, but I kept it hidden. IMO it makes sense to have a deprecated rpc call hidden as only users who were using it before should know about it and may need info, they can continue using ``help settxfee``

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840503929)
I suppose I can `addnode` my trusted node for now, but I will be leaking other outbound connections.
I'm not sure how to handle my case unless we don't consider the ephemeral broadcast connections as true "connections" in the sense of `connect` option.

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840504627)
I suppose I can `addnode` my trusted node for now, but I will be leaking other outbound connections.
I'm not sure how to handle my case unless we don't consider the ephemeral broadcast connections as true "connections" in the sense of `connect` option.

💬 m3dwards commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2473926492)
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2473947449)
> I understand you were burned by endianness but I disagree that it's worth sacrificing readability where endianness is a non-issue.

Thanks @hodlinator for the suggestions, I tried them all, but in the end decided that I value consistency more than coming up with a separate solution for each test case. These are ugly, I agree, but at least they're testing the setup we're using in prod.
I did however change the hard-coded 8 values to `sizeof xor_key` (for memcpy) or `sizeof(uint64_t)` for vec
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2473948407)
For later blocks where cache misses are much more common, this change has an even bigger impact.
This benchmark report shows a 40% speedup measuring from blocks 840k to 850k. Also, compare flamegraphs of master and this branch, where the latter has 15 worker threads fetching coins from disk.
https://bitcoin-dev-tools.github.io/benchcoin/results/pr-19/11798124132/index.html

🤔 instagibbs reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2430699423)
first pass, looks good but I want to spend more time making sure queue and control flow changes are correct

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1838775296)
note: this is removed because it's causing a 2nd error that may mask the first's error being reported. Seems like a mistake in the original test code? Agree with its removal.

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1840601923)
asserting on logging is subpar I know, but we could do tighter assertions now if we want to check we're formatting properly:
```
diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py
index 557fcc7cea..220a6f8b25 100755
--- a/test/functional/feature_cltv.py
+++ b/test/functional/feature_cltv.py
@@ -172,11 +172,13 @@ class BIP65Test(BitcoinTestFramework):
# Now we verify that a block with this transaction is also invalid.
block.vtx[1] = sp
...

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1838787662)
maybe unnecessary if the logging includes the actual block with witness commitment?

```Suggestion
const auto debug_str = strprintf("input %i of %s (wtxid %s), spending %s:%i", nIn, ptxTo->GetHash().ToString(), ptxTo->GetWitnessHash().ToString(), ptxTo->vin[nIn].prevout.hash.ToString(), ptxTo->vin[nIn].prevout.n);
```