💬 TheCharlatan commented on pull request "Drop Windows Socket dependency for `randomenv.cpp`":
(https://github.com/bitcoin/bitcoin/pull/29786#issuecomment-2033093552)
Concept ACK

This does indeed remove the dependency on ws2_32 from the kernel lib.

💬 TheCharlatan commented on pull request "Drop Windows Socket dependency for `randomenv.cpp`":
(https://github.com/bitcoin/bitcoin/pull/29786#discussion_r1548605938)
> then a test or similar should be added that actually enforces that

Could be something as simple as doing a symbol check for `bitcoin-chainstate` and ensuring `WS2_32` is not in its import table. I guess we should be doing this anyway at some point, but not sure where I would put such a check for non-release, experimental binaries.

💬 achow101 commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#issuecomment-2033146002)
ACK 2ef71c73582be554e565ada3f8a6ca77c2cd79f1

💬 achow101 commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1548649866)
> It seems to me that the `nSequence` check here is too restrictive. Locktime is disabled when _all_ sequences are set to final, but it seems to me that `can_anti_fee_snipe` would be set to `false` here, even if only one input’s sequence is set to final.

Currently `DiscourageFeeSniping` will assert if any sequence is final, so this check is appropriate for now. It also enforces that the sequences are either `MAX_SEQUENCE_NONFINAL` or `MAX_BIP125_RBF_SEQUENCE`, so this needs to be checking tha
...

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1548572949)
```suggestion
* Get the transaction that has been broadcast fewest times and least recently.
```

🤔 andrewtoth reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-1974917244)
Do we need test coverage for retrying stale txs and having multiple concurrent txs queued for private broadcast?

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1548540109)
```suggestion
* - Get a transaction for broadcast, the one that has been broadcast fewer times and least recently
```

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1548580964)
nit: This could be `const` method?

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1548596412)
Since we're touching these lines, maybe update to `LogPrintLevel`?

💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2033180778)
Thank you for taking a look @Sjors and @maflcko!

> If you run these in parallel, I think you might as well drop the `--skipunit` since it's faster than many of the other tests.

To ensure I understand, is the desire here to have the unit tests run by default when running individual tests (and also by default when running all tests)?

If so, that seems reasonable to me. As you mentioned, the unit tests are decently fast and the default job value is 4 anyway (so there would be some inheren
...

📝 achow101 opened a pull request: "[25.x] Finalize 25.2"
(https://github.com/bitcoin/bitcoin/pull/29794)
Final changes for 25.2 release

🤔 marcofleon reviewed a pull request: "cli: Detect port errors in rpcconnect and rpcport"
(https://github.com/bitcoin/bitcoin/pull/29521#pullrequestreview-1975216076)
Tested ACK. The new error handling code looks good to me upon review. Ran the daemon and gave invalid values for `rpcconnect` and `rpcport` and the proper error messages came up. The behavior of port 0 being invalid is consistent with `init.cpp`. Ran `interface_bitcoin_cli.py` too and there were no issues with the added tests.

💬 tdb3 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#issuecomment-2033263056)
> Tested ACK. The new error handling code looks good to me upon review. Ran the daemon and gave invalid values for `rpcconnect` and `rpcport` and the proper error messages came up. The behavior of port 0 being invalid is consistent with `src/init.cpp`. Ran `interface_bitcoin_cli.py` too and there were no issues with the added tests.

Thank you for reviewing, @marcofleon!

💬 pablomartin4btc commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#discussion_r1548752403)
**nit**: the tx won't be created when an error is raised during the `createTransaction`'s call (eg no recipient) so the obj should represent the correct thing, unless I misunderstanding the use case here... ok I see now this is how `src/util/result.h` works, so ignore my correction, I find it a bit weird but perhaps cos I'm not used to it yet. As a ref this change could be useful https://github.com/bitcoin/bitcoin/pull/25665
```suggestion
virtual util::Result<CreateTransactionResult> crea
...

👍 pablomartin4btc approved a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-1975255350)
cr ACK dfbd145676255f82bad529976928dff1939c5e48

💬 davidgumberg commented on pull request "lint: Rewrite spelling, includes, and include guards linters in Rust":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2033339070)
I've substantially revised this PR to address @fjahr's feedback that the rust lint runner monofile was getting too big, and that it doesn't support running individual tests (https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2022796657) and @maflcko's suggestion to include `test/lint/lint-locale-dependence.py` and `test/lint/lint-python-utf8-encoding.py` (https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2022606314).

I do not feel confident in regex, and had to do some tricky
...

💬 kevkevinpal commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1548842239)
nit: not sure if this comment is redundant now since the function name is `calculate_input_weight`

💬 kevkevinpal commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1548842910)
nit: not sure if this comment is redundant now since the function name is `calculate_input_weight`

💬 kevkevinpal commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1548845139)
It might make sense to add another test that does not explicitly set `witness_stack_hex` equal to `None`

```
self.assertEqual(calculate_input_weight(""),
(SKELETON_BYTES + SMALL_LEN_BYTES) * WITNESS_SCALE_FACTOR)
```

💬 kevkevinpal commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1548863105)
nit: we only need to define this once