💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2524678779)
re: https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2518375913

Thanks, added a more specific error message for this case.

💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2524680342)
re: https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2518380662

> nit: I was going to say that the unspecified file and the missing file test could be deduplicated since they are currently similar but I think the better change would be to have an explicit error message for the empty `-asmap` arg.

Make sense, implemented the separate error message

💬 yancyribbens commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524738408)
I was pointing out you where using `&&` when it should have been `||` since `data_dir == nullptr && data_dir_len > 0` will not return early if `data_dir` is null but `data_dir_len` is greater than zero. Looks like you made that change though :)

💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2524753751)
Shouldn't this and the one below be `LogWarning` to clearly highlight that something is wrong?

🤔 w0xlt reviewed a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3461743341)
ACK https://github.com/bitcoin/bitcoin/pull/33872/commits/0aebdac95da9a7d476264424c0107bd806ce5362

💬 w0xlt commented on pull request "init: completely remove `-maxorphantx` option":
(https://github.com/bitcoin/bitcoin/pull/33872#discussion_r2524978802)
nit:
```suggestion
previously deprecated and has no effect anymore since v30.0. (#33872).
Passing it now causes startup failure.
```

👍 TheCharlatan approved a pull request: "kernel: handle null or empty directories in implementation"
(https://github.com/bitcoin/bitcoin/pull/33867#pullrequestreview-3461861782)
ACK 6657bcbdb4d0359c1843ca31fb3670c7c0c260d5

💬 stringintech commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3529907874)
Thanks @ryanofsky for explaining the trade-offs!

I think I also prefer we push this change after we support separate logger instances per connection though, and at this point keep the API consistent with what it's currently capable of. I'm not sure how big of an impact delaying this would have on bindings/users, especially if we're assuming their use cases are mostly limited to a single logging connection.

👍 pablomartin4btc approved a pull request: "Added test coverage for qt gui#901 console history filter"
(https://github.com/bitcoin-core/gui/pull/910#pullrequestreview-3462041401)
ACK 310e4979b36cbcf1e9e01dd90c14e2e9997343a0

💬 w0xlt commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#issuecomment-3530135641)
Concept ACK

💬 Sammie05 commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#issuecomment-3530190674)
Tested on Windows and the Qt test runs successfully without requiring manual QT_PLUGIN_PATH environment setup.

Approved

💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2525371442)
> will not return early if `data_dir` is null but `data_dir_len` is greater than zero

that's literally English for `data_dir == nullptr && data_dir_len > 0` evaluating to `true`, therefore satisfying the `if` condition, therefore triggering the early return.

> you where using `&&` when it should have been `||`

Sorry, but... no. If the statement was `if ((data_dir == nullptr || data_dir_len > 0) || (blocks_dir == nullptr || blocks_dir_len > 0))` as you suggest, we'd do an early nullptr r
...

💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3530425864)
Thanks for the feedback @stringintech. Can you explain what specifically you may be looking for which this PR does not provide? The API defined here is consistent and logical. It just happens to be compatible with new features like per-connection options and separate log streams (already implemented in #30342) instead of being pointlessly incompatible with them. It also eliminates the btck_logging_disable() function which is unsafe and confusing. And it gets rid of the kernel's inefficient defau
...

💬 151henry151 commented on pull request "doc: gen-manpages.py should check build options":
(https://github.com/bitcoin/bitcoin/pull/33085#issuecomment-3530698712)
Hi @BrandonOdiwuor; I did some work on this in #33828, perhaps you might find some ideas there to implement here. Feel free to use any of it that might help.

💬 danielabrozzoni commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#issuecomment-3530733454)
post-merge concept ACK, thanks for working on this! :)

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2525677364)
Would it be simpler to not bother with the mutex and condvar here?

```diff
diff --git a/src/net.cpp b/src/net.cpp
index cd4bcf0ea3..16b2d61b60 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -3095,31 +3095,29 @@ std::optional<Network> CConnman::PrivateBroadcast::PickNetwork(std::optional<Pro

size_t CConnman::PrivateBroadcast::NumToOpen() const
{
- LOCK(m_num_to_open_mutex);
return m_num_to_open;
}

void CConnman::PrivateBroadcast::NumToOpenAdd(size_t n)
{
- WIT
...

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2525874952)
Maybe (not tested):

```cpp
int Step()
{
const int ret = sqlite3_step(m_stmt);
if (ret == SQLITE_ROW || ret == SQLITE_DONE) {
return ret;
}
sqlite3_finalize(m_stmt);
m_stmt = nullptr;
// then the destructor can assert(sqlite3_finalize(m_stmt) == SQLITE_OK);
}
```
Related to https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518504627

💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2525904085)
I see your point, but the underlying structures are different for `ScriptSig` and `ScriptWitness`: simple byte vector for script sig `ScriptSig` and vector of byte vectors for the `ScriptWitness`. So these are more accurate representatives of what is happening under the hood imo. Its also standard to print script sig like this from what i can see.

💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2525907152)
yes but its better to roll up responsibility to the object that holds it? Can you point out exactly where maybe? so we can see? if not, this seems like a better approach to me.

💬 Ataraxia009 commented on pull request "init: Changing the rpcbind argument being ignored to a pop up warning":
(https://github.com/bitcoin/bitcoin/pull/33813#discussion_r2525940186)
Not necessary since its a weird case imo? Why would you ever hit it?