Bitcoin Core Github
43 subscribers
122K links
Download Telegram
💬 theStack commented on pull request "Ephemeral Anchors, take 2":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2156706861)
Is it an option to be more restrictive and only allow zero-value outputs as ephemeral anchors, for not having to deal with the concept of dust at all? Or, asked differently: what would be the motivation/benefit for users to ever create an anchor output with nValue > 0? (Note that I haven't looked too deep into the concept of ephemeral anchors and the predecessor PR #29001, so very likely I'm missing something obvious here, but I guess the answer could be interesting for other potential reviewers
...
👍 theStack approved a pull request: "refactor: performance-for-range-copy in psbt.h"
(https://github.com/bitcoin/bitcoin/pull/30253#pullrequestreview-2106322753)
utACK fab01b5220c28a334b451ed9625bd3914c48e6af
💬 tdb3 commented on pull request "refactor: performance-for-range-copy in psbt.h":
(https://github.com/bitcoin/bitcoin/pull/30253#issuecomment-2156718032)
>no further change should be needed.

Thanks.
ACK fab01b5220c28a334b451ed9625bd3914c48e6af
💬 naiyoma commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#issuecomment-2156762313)
> Rather than checking each one of these options individually, why not have an `OptionsCategory` for them, and iterate over that? That has the advantage that it lists them separately when you invoke `bitcoin-cli -help`,

Thanks for this suggestion, I have applied the patch and pushed an update
📝 l2xl opened a pull request: "Allow to configure custom libzmq prefix"
(https://github.com/bitcoin/bitcoin/pull/30256)
This change allows to to configure bitcoin build with ZeroMQ library
installed at any place standard or CUSTOM.
This is useful if needed to use libzmq of different version from installed by distro.
📝 maflcko opened a pull request: "build: Remove --enable-gprof"
(https://github.com/bitcoin/bitcoin/pull/30257)
It is unclear what benefit this option has, given that:

* `gprof` requires re-compilation (`perf` and other tools can instead be used on existing executables)
* `gprof` requires hardening to be disabled
* `gprof` doesn't work with `clang`
* Anyone really wanting to use it could pass the required flags to `./configure`
* I couldn't find any mention of the use of `gprof` in the discussions in this repo, apart from the initial pull request adding it (cfaac2a60f3ac63ae8deccb03d88bd559449b78c)
...
👍 TheCharlatan approved a pull request: "build: Remove --enable-gprof"
(https://github.com/bitcoin/bitcoin/pull/30257#pullrequestreview-2106371803)
ACK fa780e1c25e8e98253e32d93db65f78a0092433f
🤔 furszy reviewed a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2106239011)
Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099

Great last commit.

Question about 804f09dfa116300914e2aeef05ed9710dd504e8c commit description:

> Also the previous set of
options did not allow rebuilding the block database without also
rebuilding the chainstate database, when it should be possible to do
those independently.

is this tested anywhere?
💬 furszy commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632309533)
tiny nit
```suggestion
options.wipe_block_tree_db = m_args.GetBoolArg("-reindex", false);
options.wipe_chainstate_db = options.wipe_block_tree_db || m_args.GetBoolArg("-reindex-chainstate", false);
```

Side note:
I don't think this is used anywhere.
📝 Dason13 opened a pull request: "Codespace potential funicular rqqw9x756q9cp6rw"
(https://github.com/bitcoin/bitcoin/pull/30258)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...
fanquake closed a pull request: "Codespace potential funicular rqqw9x756q9cp6rw"
(https://github.com/bitcoin/bitcoin/pull/30258)
📝 fanquake locked a pull request: "Codespace potential funicular rqqw9x756q9cp6rw"
(https://github.com/bitcoin/bitcoin/pull/30258)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...
🤔 furszy reviewed a pull request: "test: Remove redundant verack check"
(https://github.com/bitcoin/bitcoin/pull/30252#pullrequestreview-2106373554)
Code ACK fa6627706580ea with a comment.
💬 furszy commented on pull request "test: Remove redundant verack check":
(https://github.com/bitcoin/bitcoin/pull/30252#discussion_r1632399010)
It would be nice to be explicit about the race conditions rather than keeping this vague. Maybe could add something like: "The node will ignore all messages received during the version handshake window".
💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632419791)
I suggested the current approach, see https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1631387475

> I don't think this is used anywhere.

What do you mean?
🤔 theStack reviewed a pull request: "util: add BitSet"
(https://github.com/bitcoin/bitcoin/pull/30160#pullrequestreview-2106417903)
Reviewed 1e08ba37e7b5d9b083c938d90526f8fabbe1c799 so far, the code looks correct to me. Planning to review and run the fuzz test tomorrow.
💬 theStack commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1632438115)
nit: same as done in `operator++`, could add `Assume(m_val != 0)` here as well (both for `IntBitSet` and `MultiIntBitSet` iterators)
🤔 tdb3 reviewed a pull request: "cli: restrict multiple exclusive argument usage in bitcoin-cli"
(https://github.com/bitcoin/bitcoin/pull/30148#pullrequestreview-2106509791)
Looks like commits 628d2d4173660c1ad35b2d84524f71b92593d7cd and ac0d2e474b5df93c8e5824d4d326eda977aade3d overlap a bit. For example:

https://github.com/bitcoin/bitcoin/blob/628d2d4173660c1ad35b2d84524f71b92593d7cd/src/bitcoin-cli.cpp#L1178-L1182

How about squashing some of these changes to help keep the master branch commit log cleaner?
💬 furszy commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632512298)
> I suggested the current approach, see https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1631387475

Hmm ok. We should probably go further and deduplicate the init.cpp / setup_commons.cpp code somewhere in the future.

> > I don't think this is used anywhere.
>
> What do you mean?

This is part of the unit test framework and no unit test, benchmark or fuzz test seems to make use of it. "-reindex" and "-reindex-chainstate" are always unset.
📝 threewebcode opened a pull request: "chore: fix typos"
(https://github.com/bitcoin/bitcoin/pull/30259)
fix the typos in the code comments

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test
...