💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1654661980)
Will leave for now so as to not require reACKs for a house keeping change, but will reorder if I end up needing retouch. Thanks!

💬 brunoerg commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654672807)
In 341d5e1da1a40ed6dec449bc64132de5ae8788c2:
```suggestion
assert_raises_rpc_error(-5, "Block not found", node.gettxoutsetinfo, "none", "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09", True)
```

💬 Sjors commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2191508130)
> The actual sites on this domain have HSTS enabled with subdomain inclusion, although you have to visit those sites for the browser to know about that. I've also submitted it to the HSTS preload list.

Oh that's a good idea...

💬 brunoerg commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654674147)
In 341d5e1da1a40ed6dec449bc64132de5ae8788c2, I don't think this comment is necessary since the next line is clear and "self explanatory":
```suggestion
```

💬 brunoerg commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2191519784)
Since calling `gettxoutsetinfo` with block hash depends on `coinstatsindex`. It would be simpler to address it on `feature_coinstatsindex`. It would be just one line, and would avoid the node restart and the function reordering.

💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654687943)
`state` will be valid, no? So this may be confusing?

💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654690210)
just a nit, because it only affects test-only code, but wanted to leave the comment.

💬 kevkevinpal commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#issuecomment-2191547359)
ACK [e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)[e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)

💬 AngusP commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654705968)
nit: In the RPC error you'll give `state.ToString()` as the failure reason, but if this tip check fails that'll be the default `BLOCK_RESULT_UNSET`.

Seems correct to fail if the tip has updated to avoid building a fork on an older block, but I can't see a `BlockValidationResult` that would make sense here other than maybe stretching the meaning of `BLOCK_INVALID_HEADER` -- perhaps you could add another that only makes sense in a templating context like `BLOCK_TEMPLTE_NOT_TIP` or wrap `BlockVa
...

💬 AngusP commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654715485)
Currently if this happens as you said https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654637548 it looks like we'd have crashed which is fun

🤔 tdb3 reviewed a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2141646230)
Concept ACK
Left one minor comment.

💬 tdb3 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1654715835)
Looks like this is checking for a block hash that doesn't exist after block hash for block 1 is reconsidered, which makes sense. Recommend adding `self.log.info()` to make this clearer to the reader.

```python
b1hash = node.getblockhash(1)
node.invalidateblock(b1hash)
...
self.log.info("Test gettxoutsetinfo returns the same result after invalidate/reconsider block")
node.reconsiderblock(b1hash)
```

👍 tdb3 approved a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2141654158)
re ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e
Thanks for implementing the changes.

💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654751499)
> probably a bug fix.

In test-only code. But seems fine to fix, if the fix is correct.

💬 vasild commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#discussion_r1654817299)
This makes the call sites even more verbose and `LogInstance(), ` is repeated everywhere. In this case one improvement could be to use a default argument for that parameter which defaults to `LogInstance()` but I can see that this may not play well with the `...` arguments.

Maybe introduce another function that takes `(category, level, instance, ...)`? Or check if the first of `...` arguments in `(category, level, ...)` is a log instance and if not, then default the instance to `LogInstance()
...

💬 ryanofsky commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654843443)
re: https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654338332

> What is this lock used for?

Marking this thread resolved as the lock is now removed in later commit c8343e29a66082b37c37ee0ff8f7433b97eea9b1

🤔 furszy reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2141863395)
Strange CI https://github.com/bitcoin/bitcoin/actions/runs/9649050504/job/26611651920?pr=26596 error.. it doesn't seem to be related. The "salvage" command opens the wallet in `DatabaseFormat::BERKELEY` format.

💬 willcl-ark commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-2191727416)
@foolbear are you able to provide some concrete steps to reproduce this? I can't get it to happen in a few quick tests on my side.

Without reproduction steps it's likely going to be difficult to fix, and perhaps we should close the issue for now until we get verified repro steps.

👍 ryanofsky approved a pull request: "Mining interface followups"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2141854539)
Code review ACK c8343e29a66082b37c37ee0ff8f7433b97eea9b1

Nice changes getting rid of some over-broad and recursive uses of cs_main.

💬 ryanofsky commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654849168)
In commit "Drop unneeded lock from createNewBlock" (1e4918b8f3af20577acdc9f201af44153e29bcb3)

Commit message could mention the reason the lock is not required in CreateNewBlock, which is just that CreateNewBlock already locks cs_main internally.