💬 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`?

👍 TheCharlatan approved a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2060338479)
Re-ACK c0e0a0affea3e3cee9a8910de4bae55a9bdd24cf

💬 fanquake commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2114842252)
Now that the research has been done, it should be added to the commit message and PR description, so it's clear why this change might be safe, or the previous thread_local assumptions misguided. The commit message of `The previously mentioned erroneous behavior is not an issue anymore.` explains nothing.

💬 glozow commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603098402)
> although unclear to me why we don't also remove the child transaction from fee estimation to avoid overestimating

A child is already ignored, new mempool txns with unconfirmed parents are never added

https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/validation.cpp#L1274-L1279
https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/policy/fees.cpp#L614-L615

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603108416)
Thanks for your review, Josie. I think you are missing something.

When the child arrives, it will have a mempool parent, and we currently do not consider all transactions with mempool parent for fee estimation.

So when processing a transaction in `processBlockTx` all transactions with mempool parent, will be missing and , and we will skip them.

Child transactions whose parents aren't in our mempool will also be missing, and we will skip them because orphan transactions aren't added to t
...

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603130595)
Yeah, I can pick this up in a separate PR.

💬 hebasto commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2114912000)
> Now that the research has been done, it should be added to the commit message and PR description, so it's clear why this change might be safe, or the previous thread_local assumptions misguided. The commit message of `The previously mentioned erroneous behavior is not an issue anymore.` explains nothing.

Thanks! Done.