Bitcoin Core Github
42 subscribers
126K links
Download Telegram
📝 theStack opened a pull request: "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)"
(https://github.com/bitcoin/bitcoin/pull/32690)
This PR fixes the multiprocess depends build for OpenBSD by applying upstream patch https://github.com/capnproto/capnproto/pull/2308 and switching the SHA256SUM command to output hash sums in the expected format (the default is BSD format [1], but we need GNU format [2], see commit message for details).

The first commit can be replaced with a simple capnp version bump once this is available in a release.

Tested on OpenBSD 7.7 (x86_64) via
```
$ gmake -C depends MULTIPROCESS=1 NO_BOOST=1
...
💬 theStack commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2947231102)
> [capnproto/capnproto#2308](https://github.com/capnproto/capnproto/pull/2308) - has landed, so you can pull that in here.

Opened #32690 since it turned out that only applying the patch was not enough, as another issue regarding the SHA256SUM command was revealed.
🤔 davidgumberg reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2901675322)
ACK https://github.com/bitcoin/bitcoin/pull/27286/commits/39734259fc10b1339db2ae80c5fab12916cf0aff

Left a lot of non-blocking nits/questions/observations, many of which are out-of-scope, and all of which (except for commit hygiene nits) can be addressed in a follow up.

Benchmarked this branch with the hard-coded `nEpochIterations` removed as suggested by @rkrux (https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2850393076) against master with this commit cherry-picked on top,
...
💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2129798416)
In https://github.com/bitcoin/bitcoin/pull/27286/commits/41c82afdfc94d7e0e1daa5ac4f4e5814e113c398 (_wallet: Keep track of transaction outputs owned by the wallet_)

Feel-free-to-disregard-big-picture-out-of-PR-scope-question-comment:

I feel like one of the challenges in reading `CWallet` is how much state exists, and how many places that state is modified/read/accessed from, maybe most of this is unavoidable, but I wonder if it would be a benefit to have some of these wallet state members
...
💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2130604690)
https://github.com/bitcoin/bitcoin/pull/27286/commits/ae50dcb3a49eaf40fb64fd227c447ba90c45db34 (_wallet: Change balance calculation to use m_txos_)

---

nit: here and above, I think `GetTxOut()` and `GetWalletTx()` are used enough to justify a variable, also minimizes diff if names are reused, but I'm ~0 on changing them:

```suggestion
for (const auto& [outpoint, txo] : wallet.GetTXOs()) {
const CWalletTx& wtx{txo.GetWalletTx()};
const CTxOut& output{tx
...
💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2130659438)
https://github.com/bitcoin/bitcoin/pull/27286/commits/ae50dcb3a49eaf40fb64fd227c447ba90c45db34 (_wallet: Change balance calculation to use m_txos_)

nit: moving this check up would make the diff smaller
💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2130634083)
ae50dcb3a49eaf40fb64fd227c447ba90c45db34 (_wallet: Change balance calculation to use m_txos_)

Nit that can be addressed in a follow up, this commit leaves `ISMINE_USED` and related logic as dead code.
💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2129872942)
In commit https://github.com/bitcoin/bitcoin/pull/27286/commits/ad76a44c3d9f300d5d230af442d36f90b349bc12 (_wallet: Recompute wallet TXOs after descriptor migration_)

nit: This commit should be moved to before `m_txos` is used in `GetBalance()`
💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2129958056)
https://github.com/bitcoin/bitcoin/pull/27286/commits/8d9c7c3461d088211dde0551909898b8018059bf (_wallet: Recalculate the wallet's txos after any imports_)

----
Just an observation:

It's interesting to me that it's expected behavior that even without a rescan, previously untracked outputs of tracked transactions that become spendable are known to the wallet. I suppose this is a side effect of the logic before this PR, and this PR preserves that behavior. This is tested for in the earlier h
...
💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2130703872)
```suggestion
if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && txo.GetWalletTx().isConfirmed() && tx_depth >= min_depth) {
```

or alternately, nesting the immature and trusted checks together, maybe like:

```cpp
if (is_trusted && tx_depth >= min_depth) {
if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && txo.GetWalletTx().isConfirmed()) {
ret.m_mine_immature += credit_mine;
ret.m_
...
💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2130512343)
https://github.com/bitcoin/bitcoin/pull/27286/commits/ae50dcb3a49eaf40fb64fd227c447ba90c45db34 (_wallet: Change balance calculation to use m_txos_)

----
Why is this check added?
💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2130673491)
Out of scope observation:

It seems strange that there isn't a balance type for outputs that are trusted and confirmed in a block, but below `min_depth` confirmations, you see some balance when the tx is in the mempool, and then when it's got enough confirmations, but in between it disappears from your balances seems odd to me.
💬 why19850 commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-2947734879)
1CKk5YAEqzrsTci9iPCoahnZ2dbEnyWZRi
💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2131471717)
Ping @TheCharlatan @maflcko :)
💬 i-am-yuvi commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#issuecomment-2948079431)
Concept ACK

Great observation and thanks for adding. I was thinking if we could add this timeout test in [`p2p_initial_headers_sync.py`](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_initial_headers_sync.py) instead of creating another one.
💬 i-am-yuvi commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#issuecomment-2948151373)
ACK fac00d4ed361e5e8c8989b2bb5a4a22dd54e2c72

Thanks for the cleanup. The minor suggestion regarding linter instructions is noted and can be addressed in a future update.
💬 Nicholasdieringer-creator commented on issue "avoid using invalidateblock to directly test reorg behavior":
(https://github.com/bitcoin/bitcoin/issues/32531#issuecomment-2948204392)
Hello so I'm new at coding and find AI fascinating. I'm trying to help build open source and platforms to contribute to Ai and bring Innovation to health mental health and Discovery and technology. I'd love to contribute. I'm new at this and I'm learning. It's exciting and I definitely have some amazing ideas, a powerful vision and incredible insight in learning all about this and being a part of the ecosystem. As I said I'm new so I would love the opportunity to help anyway I can, and earn the
...
💬 maflcko commented on pull request "ci: Rewrite test-each-commit as py script":
(https://github.com/bitcoin/bitcoin/pull/32680#issuecomment-2948223714)
Hmm, yeah the boilerplate overhead is a bit unfortunate. I switched to python, which still has downsides such as https://github.com/bitcoin/bitcoin/pull/29068#issuecomment-1852746032, but it seems manageable here. Also, updated the pull description.

I think the danger of Bash, that it looks harmless and superficially simple and correct, but in many such "harmless" constructs can silently drop a failing return status is reason enough to prefer a slightly more verbose language. Maybe rust is to
...
💬 Muniru0 commented on issue "doc: references to unshipped documentation":
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2948296702)

> > [@fanquake](https://github.com/fanquake) why not just updating the the docs to point to the online versions. e.g:
> > **Replace:** `see doc/cjdns.md`
> > With: `see https://github.com/bitcoin/bitcoin/blob/master/doc/cjdns.md`
> > This make's sure that:
> >
> > 1. **Accessibility**: Users can easily access the documentation regardless of how they obtained the binaries.
> > 2. **Up-to-Date Information**: Linking to the online repository ensures that users are directed to the most current ver
...
💬 Muniru0 commented on issue "doc: references to unshipped documentation":
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2948298581)
@Zeegaths check I hope you check the comment from @brunoerg before proceeding.
🤔 vasild reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2903739719)
Approach ACK 7d301184016a3f59c2e363dff631263cdbe21da0