⚠️ jplix7k opened an issue: "Wrong block mined time in testnet"
(https://github.com/bitcoin/bitcoin/issues/30121)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Currently, all block mined time in testnet could be about 1 hour earlier than current time.

<img width="1285" alt="testnet" src="https://github.com/bitcoin/bitcoin/assets/51077262/4df52a40-7b45-4a4d-8535-f1564e9a8705">
link: https://blockexplorer.one/bitcoin/testnet

Timestamp in UpdateTip is wrong.
```
│ app-generic 2024-05-16T06:57:48Z Saw new header hash=000000000000000ba2e091
...

🤔 real-or-random reviewed a pull request: "Update libsecp256k1 subtree to current master"
(https://github.com/bitcoin/bitcoin/pull/30120#pullrequestreview-2059788830)
Concept ACK

@hebasto Or would you prefer waiting for https://github.com/bitcoin-core/secp256k1/pull/1529?

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1602742155)
It's hidden as debug because at the moment this is shown when the user doesn't have IPv6. Could boost this to warning if we do the address check first, then don't bother looking for a default gateway if there's none. Will do that.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1602763394)
Whoops, yes, it's `setEnabled`.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1602770267)
Will keep this open but leave it as-is for now. For IPv4 i'm not currently sure how to check if we have (Internet) networking besides checking for a default gateway, and i'd like to keep the two paths reasonably symmetric. Just checking for publicly routable addresses isn't going to cut it for IPv4.

💬 remyers commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2114278531)
I changed `disable_algos` to `enable_algos` in [3154f83](https://github.com/bitcoin/bitcoin/pull/30080/commits/3154f833d78c3863842046a61fc3270aaac18ce8). This was suggested by @t-bast because it won't cause problems for existing users when new cs algorithms are added. I believe I have the simplest and safest solution for initializing the `m_enable_algos` `std::bitset` to all true, but there are a few other ways to do it.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2114295253)
- Rebased for a trivial merge conflict related to clang installation in `.github/workflows/ci.yml`.
- Addressed a few comments.

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1602816185)
Alternatively I can add a commit that removes the `TxOrphanage` lock?

💬 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).