💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1602821815)
Good idea. Though it seems that this doesn't apply anymore since the commit is already merged, so resolving.

💬 ajtowns commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1602847832)
I expect just removing it would be fine and easy, but no need to bump it up higher in the todo list

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1602865868)
> I've summarized my understanding into the below

Looks correct to me

> I think the fact that we need such a verbose comment is a pretty strong sign AlreadyHaveTx should be refactored, e.g. using {Wt,T}xid types as I suggested https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587694422, but I appreciate that it's mostly orthogonal to this PR.

I think it's really a sign to ban txid-based transaction relay. Just kidding. I agree it's hard to reason about logic dealing with a hash
...

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2114500633)
All outstanding TODOs were done. Rebased to master, collected fixups.

💬 laanwj commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2114646750)
`getrpcversion` maybe? The reason i bring it up is that it needs to be restrictive enough in scope to just the RPC interface version, not wallet version, not P2P versions, etc.

👍 rkrux approved a pull request: "Fee Estimation: Ignore all transactions with an in-block child"
(https://github.com/bitcoin/bitcoin/pull/30079#pullrequestreview-2060133382)
reACK [2563305](https://github.com/bitcoin/bitcoin/pull/30079/commits/2563305c0aef3975a6321911db7e0f2a245486de)

Thanks for the quick turnaround on this @ismaelsadeeq!

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1602965411)
`Decimal(parent_tx["tx"].vout[0].nValue) / COIN`

Is there not a `satToBtc()` already? WDYT about introducing one? I've noticed this conversion in this diff thrice.

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1602989484)
Good idea for a follow-up @rkrux

💬 josibake commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2114713806)
reACK https://github.com/bitcoin/bitcoin/pull/26606/commits/c0e0a0affea3e3cee9a8910de4bae55a9bdd24cf

💬 laanwj commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1603033599)
Adding a comment would make sense. Storing a POD data structure in thread-local data instead of a C++ object that allocates on the heap is simpler and more widely supported.
Please also mention that this should be the only use of thread-local data in the program.

🤔 josibake reviewed a pull request: "Fee Estimation: Ignore all transactions with an in-block child"
(https://github.com/bitcoin/bitcoin/pull/30079#pullrequestreview-2060232786)
Looks good, although unclear to me why we don't also remove the child transaction from fee estimation to avoid overestimating. Would recommend also removing the children, or at least documenting why it doesn't matter in a comment.

💬 josibake commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603032016)
in https://github.com/bitcoin/bitcoin/pull/30079/commits/a6e81336897bfa472b79c82899584b2763af99e4 ("ignore all transaction with in block child"):

nit: `/** comment */` to match the styling of the surrounding comments. Might also be worth mention this is only for in-block children (CPFP).

💬 josibake commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603028373)
in https://github.com/bitcoin/bitcoin/pull/30079/commits/a6e81336897bfa472b79c82899584b2763af99e4 ("ignore all transaction with in block child"):

Shouldn't we also remove the child? Otherwise we will overestimate fees, unless I'm missing something? Example:

* Parent pays 1 sat/vb
* 10 sat/vb per tx is required to be competitive for the next block
* Child pays 20 sat/vb to try and get them + parent to 10 sat/vb
* Now in fee estimation, we see 20 sat/vb for a child that *could* have only
...

💬 josibake commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603035220)
yeah, would recommend this be a separate PR, since there are likely multiple instances in other tests where we could replace the satToBtc conversion with a utility function in the test framework.

💬 hebasto commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2114790520)
@dominicusadinfinitum

https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2090005933 ?

✅ laanwj closed a pull request: "test: Add large aligned vmov check for mingw"
(https://github.com/bitcoin/bitcoin/pull/29874)

💬 laanwj commented on pull request "test: Add large aligned vmov check for mingw":
(https://github.com/bitcoin/bitcoin/pull/29874#issuecomment-2114791346)
Closing for now.

📝 fanquake locked a pull request: "lint: fixed typo in test_runner causing linter warning"
(https://github.com/bitcoin/bitcoin/pull/30106)
introduced in https://github.com/bitcoin/bitcoin/commit/357ad110548d726021547d85b5b2bfcf3191d7e3

This typo is causing an error in the linter, fixing this should remove this warning

```
test/functional/test_runner.py:651: insuffient ==> insufficient
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```

[link to cirrus ci warning](https://cirrus-ci.com/task/5926490804584448?logs=lint
...

🤔 rkrux reviewed a pull request: "test: Added test to ensure log and failure happen when work is less than active chainstate"
(https://github.com/bitcoin/bitcoin/pull/30105#pullrequestreview-2060313054)
Make successful, all functional tests also. Asked a question and provided a suggestion.

@kevkevinpal Should we get rid of this TODO on line 26 now?
https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_assumeutxo.py#L26

💬 rkrux commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#discussion_r1603079655)
@kevkevinpal We are loading the snapshot of `n0` in `n0` itslelf? Should it not be loaded into `n1`?