💬 jonatack commented on pull request "ci: Add test-each-commit task":
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1682596724)
Concept ACK if our CI infra can handle it. I build and run the unit tests on each commit for more critical pulls, or pulls with commits that aren't clearly separate or that look interactive, or those with many commits, but often without also running the functional tests that take longer to run. Having the CI verify this would alert PR authors more quickly and save developer time for everyone.

💬 apoelstra commented on pull request "wallet: add `seeds` argument to `importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/27351#issuecomment-1682598823)
Rebased.

💬 jonatack commented on pull request "ci: Add test-each-commit task":
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1682600172)
> often without also running the functional tests that take longer to run

That said, what the build testing most often finds is a failing build before running any tests. So if infra resources are limited, just build + units might still catch most of the issues.

💬 nerd2ninja commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1682605147)
Concept Ack. To quote moonsettler from a different mempool policy discussion

https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666604137

>evil mempool is among the largest existential threats to bitcoin and we absolutely have no way to stop it from coming into existence. in fact unnecessarily restrictive mempool policy naturally incentivizes it's emergence.

>people attempting to effectively filter economically motivated, paying market for block space will most likely result in
...

💬 kevkevinpal commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#issuecomment-1682610519)
rebased to [5fa09d0](https://github.com/bitcoin/bitcoin/pull/28101/commits/5fa09d039f4e920c18f089fcb1fb4df7813468fc)

🤔 jonatack reviewed a pull request: "doc: clarify cookie generation in JSON-RPC-interface.md"
(https://github.com/bitcoin/bitcoin/pull/28283#pullrequestreview-1583026183)
Concept ACK

💬 jonatack commented on pull request "doc: clarify cookie generation in JSON-RPC-interface.md":
(https://github.com/bitcoin/bitcoin/pull/28283#discussion_r1297476742)
```suggestion
- **Secure authentication:** By default, when no `-rpcpassword` configuration option is specified, Bitcoin Core generates unique
```

----

Unrelated, is this still planned?

`src/httprpc.cpp:251: LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");
`

💬 jonatack commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1297489117)
If you retouch, the transition to severity-based logging will be easier if used when adding new ones. Probably choose between error, warning, or info level here; all 3 will be logged unconditionally by default, unless the user sets a custom severity level.

```suggestion
LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "%s\n", "test"); });
LogPrintLevel(BCLog::BLOCKSTORAGE, "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new bl
...

💬 jonatack commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1297490048)
Idem as https://github.com/bitcoin/bitcoin/pull/27866/files#r1297489117 if you retouch.

💬 sr-gi commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1682645449)
> Maybe the test? Though, I haven't looked at it recently.

That may make sense. I'll take a look and rebase

📝 MarcoFalke opened a pull request: "ci: Disable --coverage temporarily"
(https://github.com/bitcoin/bitcoin/pull/28285)
Looks like this causes all CI builds to be red, and doesn't work anyway, see https://github.com/bitcoin/bitcoin/issues/27593 . Temporarily disable it to allow for more time to rework it from scratch.

💬 MarcoFalke commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#issuecomment-1682662462)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 jonatack commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1297514131)
This is already included 3 lines higher.

💬 jonatack commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1297514797)
```suggestion
constexpr int DEFAULT_TOR_CONTROL_PORT = 9051;
```

📝 crywolf opened a pull request: "test: Minor fix in test - locale in terminal"
(https://github.com/bitcoin/bitcoin/pull/28286)
Running tests with non-English locale set in Linux terminal makes system tests (`system_tests/run_command`) fail. Setting the locale in failing test explicitly to 'C' fixes the failure.

_How to reproduce test failure:_
Setting LC_ALL=cs_CZ.UTF-8 (`export LC_ALL=cs_CZ.UTF-8`) in terminal and then running `make check` should be enough.

_Fix:_
My terminal is by default set to non-English locale and the test failed because it checks for English error message, but the terminal produces error
...

💬 kevkevinpal commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#issuecomment-1682682090)
> Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

rebased properly in [5d94b1f](https://github.com/bitcoin/bitcoin/pull/28101/commits/5d94b1faecd8fc98017510d86f6ea5dc002ec0a3)

💬 kevkevinpal commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1297526595)
thanks, addressed in [5d94b1f](https://github.com/bitcoin/bitcoin/pull/28101/commits/5d94b1faecd8fc98017510d86f6ea5dc002ec0a3)

💬 crywolf commented on pull request "test: Minor fix in test - locale in terminal":
(https://github.com/bitcoin/bitcoin/pull/28286#issuecomment-1682682796)
During the workshop wit Gloria @glozow on Dev/Hack/Day (part of BTC Prague conference) my tests failed. Later that day I found what the problem was and now I am finally presenting my fix as an attempt to make my first PR in this repo.

💬 kevkevinpal commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1297527127)
thanks didnt know that fixed in

[5d94b1f](https://github.com/bitcoin/bitcoin/pull/28101/commits/5d94b1faecd8fc98017510d86f6ea5dc002ec0a3)

💬 jonatack commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1297532538)
Second sentence seems redundant:
```
$ ./src/bitcoind -h | grep -A3 torcontrol
-torcontrol=<ip>:<port>
Tor control host and port to use if onion listening enabled (default:
127.0.0.1:9051). If no port is specified, the default port will
be used (default: 9051).
```

```suggestion
+ argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control host and port to use if onion listening enabled (default: %s). If no port is specified, the default port of %i will
...