💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208503121)
nit: parameter names are defined per RPC, so assuming it's called "wallet_name" is not ideal, even if at the moment it seems that the assumption is correct for all RPCs (as far as I can see). I don't have a better solution yet, but I'll think about it more.

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2204543796)
It seems like our usual way to test this is to use the RPC name as the search text. I also don't think we need to be overly verbose in logging every single statement. Could simplify this test to:

```py
def test_required_args(self, node):
self.log.info("Test that required arguments must be passed")
assert_raises_rpc_error(-1, "getdescriptoractivity", node.getdescriptoractivity)
assert_raises_rpc_error(-1, "getdescriptoractivity", node.getdescriptoractivity, [])
...

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2204560989)
I don't really see the benefit of replacing these brief and easy-to-read definitions with a variable that needs to be inspected out-of-line, especially since descriptions may need to be tailored to the specific RPC functionality. Would prefer keeping those 2 as-is?

Also nit: I find `output_descriptor_obj` a better name than `obj_with_output_descriptor`

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208893832)
I think we can avoid using raw pointers and move to a more idiomatic interface by updating the arg helpers to use string_view instead of string, making it trivially copyable. I've implemented that in #32983. Shouldn't block progress - but if it gets merged first, it might be worth adopting that interface here too.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208955963)
> Would prefer keeping those 2 as-is?

Ok, I did it that way just for reuse purpose, to avoid having a typo on the strings and so on.

`obj_with_output_descriptor` comes from the definition: "An object with output descriptor and metadata", but I'm ok with `output_descriptor_obj`.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208959369)
I thought to use `<std::string_view>` too when I did the `TMPL_INST` but didn't go forward. We can keep it this way if this gets merged first or if it does yours I can incorporate it here yeah. I'll try to review your PR tmw. Thanks!

📝 sr-gi opened a pull request: "Adds transaction propagation information to mempool transactions"
(https://github.com/bitcoin/bitcoin/pull/32986)
Currently, we store the time transactions enter the mempool (in `CTXMempoolEntry`) as a timestamp with second precision.

This patch stores data more precisely (microsecond granularity), as well as optionally storing the timestamp of the first inventory message received about each transaction in the pool, while still maintaining the previous interface for backwards compatibility (`nTime` is unchanged, and `nTimeUs` and `nFirstInvTime` are added with microsecond precision).

This is really u
...

💬 sr-gi commented on pull request "Adds transaction propagation information to mempool transactions":
(https://github.com/bitcoin/bitcoin/pull/32986#issuecomment-3076456661)
I'm currently seeking conceptual feedback. While I believe this is very useful for measuring transaction propagation and evaluating network improvements, I'm unsure whether the added complexity is justified, given that it's unlikely to be used beyond that specific context.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2209122103)
I haven't chosen wallet_name to match the RPC parameter. The function is meant to ensure consistency between two potential sources of a wallet name—the one in the endpoint and the one provided explicitly—regardless of where the second one comes from. So `wallet_name` here refers to the generic concept of a wallet name, not necessarily a parameter (RPC?) with that specific name.

That said, I see your point that parameter names vary between RPCs, and if a future RPC used this function with a di
...

💬 Ezerode commented on issue "Internal bug detected: FinalizeAndExtractPSBT(psbtx_copy, mtx)":
(https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3076686230)
0xc76c95a85BB94257dE19D6007C329EC3e40b5E5C
Ayudenme colaborandome. Mi padre piensa que es todo Scam,
me gustaría formar parte del protocolo BTC, También aconsejando cosas, como puedo hacer ?

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-3076765123)
I prefer to always check whether a syscall failed even if it _seems_ that it should always succeed. In this particular case, the _"seems"_ is not very strong because `close` is doing [more things](https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2161473468) than `flush` or `truncate` and those things could fail. So I think the current code is solid. I do not see a value in removing a check whether `close` failed.

📝 maflcko opened a pull request: "init: [gui] Avoid UB/crash in InitAndLoadChainstate"
(https://github.com/bitcoin/bitcoin/pull/32987)
`InitAndLoadChainstate` is problematic, when called twice in the GUI. This can happen when it returns a failure and the user selects an interactive reindex.

There are several bugs that have been introduced since the last time this was working correctly:

* The first one is a crash (assertion failure), which happens due to a cached tip block in the notifiications from the previous run. See https://github.com/bitcoin/bitcoin/pull/31346#discussion_r2207914726
* The second one is UB (use-after
...

✅ maflcko closed a pull request: "test: Check that the GUI interactive reindex works"
(https://github.com/bitcoin/bitcoin/pull/32979)

💬 maflcko commented on pull request "test: Check that the GUI interactive reindex works":
(https://github.com/bitcoin/bitcoin/pull/32979#issuecomment-3076792505)
fix (and this test) in https://github.com/bitcoin/bitcoin/pull/32987

👍 rkrux approved a pull request: "wallet: Set migrated wallet name only on success"
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3023217816)
lgtm ACK 8a4cfddf23a4575a1042dfa97d3478727775e8dd

Makes sense to add this.

I recall thinking once about adding something like this when I was working on #32758, but I forgot later.

💬 romanz commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2209316365)
nit: `items` size will be the total number of this block's inputs (not its transactions' count), right?

💬 romanz commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2209321522)
Looks similar to the code above - maybe refactor into a helper function?

💬 romanz commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2209333265)
Consider using `FlatFilePos` here (which contains 2 `int`s) instead of `CDiskTxPos` (which contains also the block's offset, taking an additional `int`).

💬 romanz commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2209336131)
Do we need the header's data?

💬 maflcko commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2209414266)
nit: Using uint32_t and something like this should fix the linker error:

```
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 5da02b4df4..0604dec2dc 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -731,6 +731,7 @@ TMPL_INST(CheckRequiredOrDefault, const UniValue&, *CHECK_NONFATAL(maybe_arg););
TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool(););
TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
TMPL_INST(CheckRequired
...