💬 jonatack commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1765348063)
nit if you retouch: `s/raise/raises/` and perhaps explain the difference between the failing filename passed and the wallet name
```suggestion
self.log.info('Test -generate -rpcwallet=<filename> ("wallet.dat" instead of "wallet") raises RPC error')
```

💬 maflcko commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2358889569)
> A better test would be to reproduce this with regtest which I'll create a script to demonstrate soon.

Did you get a chance to write the reproducer?

> But at the same time it seems there should be a mutex around the chain tip. If there isn't this is basically guaranteed to happen. I haven't looked yet.

Yes, the mutex is called `cs_main` or `ChainstateManager::GetMutex()` . The mutex is held while enqueueing the event. And `getblocktemplate` takes the mutex at the start.

So there may
...

💬 jonatack commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2358893398)
@pablomartin4btc I think you need to update the PR description as well.

✅ jarolrod closed an issue: "cmake: adjust devtools scripts to work under cmake build environment"
(https://github.com/bitcoin/bitcoin/issues/30923)

💬 jarolrod commented on issue "cmake: adjust devtools scripts to work under cmake build environment":
(https://github.com/bitcoin/bitcoin/issues/30923#issuecomment-2358904932)
documentation changes in the readme are sufficient imo, closing as is.

💬 mcelrath commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2358912616)
Not yet. I've been running my morning script all week and haven't seen it
happen again.

On Wed, Sep 18, 2024, 12:14 PM maflcko ***@***.***> wrote:

> A better test would be to reproduce this with regtest which I'll create a
> script to demonstrate soon.
>
> Did you get a chance to write the reproducer?
>
> But at the same time it seems there should be a mutex around the chain
> tip. If there isn't this is basically guaranteed to happen. I haven't
> looked yet.
>
> Yes, the mutex i
...

👍 jarolrod approved a pull request: "doc: fixed inconsistencies in documentation between autotools to cmake change"
(https://github.com/bitcoin/bitcoin/pull/30875#pullrequestreview-2313211226)
ACK a9964c04447745435747d9cc557165c43902783b

Tried to find other inconsistencies, but couldn't.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1765381096)
Thanks, yeah it's a bit too implicit so I added `assert_equal(len(node.getorphantxs()), 1)` to make it clearer and more explicit.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1765381138)
Seemed reasonable to add a check `assert not node.tx_in_orphanage(tx_child_2["tx"])` rather than a new method, but happy to be convinced otherwise.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1765381201)
Thanks. Increases clarity. Updated.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1765381277)
"bytes" seems clearer to me as well. Updated.

Originally "size" was chosen because it aligns with `getrawtransaction`.
If there's friction with using "bytes", I can change it back to "size".

> Argument and field naming: please consider whether there is already a naming style or spelling convention in the API for the type of object in question (blockhash, for example), and if so, try to use that

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#rpc-interface-guidelin
...

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1765381344)
Agreed. Updated.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1765381413)
Removed.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1765388442)
Thanks. Increases clarity. Updated.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2358946739)
Thanks for the review @glozow!

💬 hebasto commented on issue "Trying to run bitcoin qt on Windows and getting an AV":
(https://github.com/bitcoin/bitcoin/issues/30825#issuecomment-2358955164)
The reason of the issue is described in this comment: https://github.com/bitcoin/bitcoin/blob/69409bc6e55848cf959ed924bcc722997455d76f/src/qt/test/CMakeLists.txt#L47-L48

And a mitigation follows: https://github.com/bitcoin/bitcoin/blob/69409bc6e55848cf959ed924bcc722997455d76f/src/qt/test/CMakeLists.txt#L49-L51

That's why the issue is [not observable](https://github.com/bitcoin/bitcoin/issues/30825#issuecomment-2338560259) when running `ctest`.

👍 tdb3 approved a pull request: "doc: fixed inconsistencies in documentation between autotools to cmake change"
(https://github.com/bitcoin/bitcoin/pull/30875#pullrequestreview-2313277317)
ACK a9964c04447745435747d9cc557165c43902783b

💬 jonatack commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2359007245)
PR description much improved now, thanks.

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765436227)
In practice, this will be used to either remove a set of conflicts (together with all their descendants), or to remove a set of mined transactions (together with all their ancestors). In both cases, no transactions will remain which are both ancestors of some deleted transactions and descendants of other deleted transactions.

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765437954)
ah, right! I hadn't considered the mining side. Maybe leave something to that effect?