💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1579854025)
You probably meant `onion_arg != "0"`

🤔 mzumsande reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2020968807)
Did another round of review.

nit: `local tx relay` -> `private broadcast` in last commit message

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1580149990)
This class is getting pretty large in the end, maybe put it into its own module (similar to `TxReconciliationTracker` or `TxRequestTracker`)

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1579852847)
I think this requires an update to `tor.md`, which currently says:
`-onion=ip:port Set the proxy server to use for Tor onion services. You do not need to set this if it's the same as -proxy.`
After this PR, specifiying it explicitly is necessary if you want to do private broadcast to clearnet peers.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1580200865)
I don't think is needed, `MEMPOOL` is never tested.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1580205599)
sendrawtransaction doc should be updated (it already mentions privacy issues).

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1578537557)
have you considered using `SoftSetBoolArg` to set `-walletbroadcast` to 0 if `-privatebroadcast` was chosen?
It's not the most user-friendly thing to introduce a new command arg that would only work if another arg was also changed from its default.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1580115581)
It looks like we make sure in `net_processing` to not send any of theses messages, and here we check again. I haven't thought about this long, but can this log be triggered, or could this be an `Assume(false)` instead?

💬 davidgumberg commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1580221364)
On second thought it seems a bad idea to depend on the order in which tests are called for deciding whether or not to perform cleanup. I think that might be kind of a footgun for anyone modifying the test in the future

💬 davidgumberg commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2078316239)
ACK https://github.com/bitcoin/bitcoin/pull/29122/commits/be2c5510521081119f62ef3b29f76bb9097d0f34
Re-reviewed code, and attempted to break tests by making `ConsiderEviction` misbehave.

It is worth noting that these tests will not catch if outbound eviction logic gets triggered too early. e.g.:

```cpp
/** How long to wait for a peer to respond to a getheaders request */
static constexpr auto HEADERS_RESPONSE_TIME{0s};
// [...]
/** Timeout for (unprotected) outbound peers to sync to ou
...

:lock: fanquake locked an issue: "Bitcoin with horcrux and powmem?"
(https://github.com/bitcoin/bitcoin/issues/29964)

📝 davidgumberg opened a pull request: "Lint: support running individual rust linters and improve subtree exclusion"
(https://github.com/bitcoin/bitcoin/pull/29965)
This PR improves subtree exclusion by rewriting lint checks to reuse common exclusion logic. Most lint checks previously did not exclude `crypto/ctaes`, the locale linter did not exclude `src/crc32`

Makes the include guards lint check print all missing include guards before exiting.

Adds support for running individual linters by passing `--lint=LINT_CHECK_TO_RUN` to the lint runner. If using cargo run arguments can be passed e.g.:

```bash
cd test/lint/test_runner && cargo run -- --lint
...

💬 davidgumberg commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2078380040)
@maflcko @fjahr Thank you for the feedback, sorry for the confusion on this PR. I've opened https://github.com/bitcoin/bitcoin/pull/29965

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078452007)
Rebased 88aa093356ff142efe60df989323eb1ed1aa9e18 -> 0cbcb66257db65f0a68d82d17a2d1a83d8654b38 ([`pr/saferesult.5`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.5) -> [`pr/saferesult.6`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.5-rebase..pr/saferesult.6)) dropping Result::Update() method in this PR, and last 2 uses of it.

re: https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078213
...

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-2078502542)
> Throw `DISABLED_OPCODE` error for op_ver and its variants as it's known but just disabled opcodes. Currently, interpreter throws `BAD_OPCODE` which contains the message of `"Opcode missing or not understood"`, which isn't accurate and confusing.

Apologize for poor description, I just re-write to make it simple and clear.
I agree that this one is practically doesn't affect any, as only a few will push OP_VER opcodes on stack.
It's such a minor fix, and when I just found out that OP_VER thr
...

💬 fanquake commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2078569791)
Seems like there's at least something which could/should be backported here? Maybe only the other change, given the newly broken code has only existed in master?

✅ maflcko closed a pull request: "script: throw disabled err for op_ver and its variants"
(https://github.com/bitcoin/bitcoin/pull/28169)

💬 maflcko commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-2078663017)
> It's ok to close this PR if that is better.

Ok, I'll close it for now. When there is need, it can be re-opened later or picked up again. Thanks for working on this for such a long time.

📝 maflcko opened a pull request: "test: Add two more urlDecode tests"
(https://github.com/bitcoin/bitcoin/pull/29967)
Trivial follow-up after https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1579216072

💬 maflcko commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2078730201)
The Win64 GHA test failure looks unrelated. If you want, you can split up 54b2115bb848e1aad8a86aae858de63a851235ce into its own pull request, as it is an independent improvement. But up to you.