Bitcoin Core Github
42 subscribers
126K links
Download Telegram
⚠️ helpau opened an issue: "[CI/CD]Release channels?"
(https://github.com/bitcoin/bitcoin/issues/29446)
### Please describe the feature you'd like to see added.

Does it make any sense to use release channels(canary/beta/stable)? I don't have a full understanding of this feature for different users, but I think it might help developers of the core on Bitcoin Core software(Umbrel, Lightning nodes). Of course with a warning that this is a build for testing only.

### Is your feature related to a problem, if so please describe it.

_No response_

### Describe the solution you'd like

_No response_

#
...
💬 fjahr commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1951342477)
> Just checked it, @fjahr. I think `assert_debug_log` is slowing it down. Can you please check with the following diff?

Yeah, with that change it doesn't slow down anymore!
🤔 sdaftuar reviewed a pull request: "policy: enable sibling eviction for v3 transactions"
(https://github.com/bitcoin/bitcoin/pull/29306#pullrequestreview-1887268593)
I was thinking this PR is ready to leave Draft status -- is there anything you're waiting on before doing that?

I'm not sure how important it is to return the v3 error messages if sibling RBF fails. It seems to me that if we just declare that attempting sibling eviction is part of the v3 validation rules, then we should failure to get in due to insufficient fees as an RBF failure and not as a v3 failure. (It would also simplify the implementation slightly, so it's possible I'm slightly blin
...
💬 sdaftuar commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1493792426)
This should be impossible right? Maybe just an `Assume()` to check that there's no sibling eviction going on in this scenario?
💬 fjahr commented on pull request "Improve readability of numeric literals in consensus parameters and network settings":
(https://github.com/bitcoin/bitcoin/pull/29444#issuecomment-1951378674)
Hi @paplorinc, I'm not convinced we need to change the formatting in these particular places. We usually do such refactorings when the code in question is being touched in a PR anyway, otherwise we try to avoid the noise. But I would support it if you open a PR recommending the use of numeric literals in our coding style guidelines (`doc/developer-notes.md`).
🤔 fjahr reviewed a pull request: "rpc: Fixed signed integer overflow for large feerates"
(https://github.com/bitcoin/bitcoin/pull/29434#pullrequestreview-1887282990)
utACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493
💬 fjahr commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1493803557)
nit: Would have named it `ParseReasonableFeeRate` or `ParseLimitedFeeRate` to make clear it's doing more than just parsing.
💬 fjahr commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1493803772)
Alternatively could have made the limit a parameter, because that also makes it clearer.
💬 paplorinc commented on pull request "Improve readability of numeric literals in consensus parameters and network settings":
(https://github.com/bitcoin/bitcoin/pull/29444#issuecomment-1951389466)
Hey @fjahr, thanks for the quick feedback.

I have targeted these numbers, since they're quoted to normies all the time when asking "so how do you know there isn't an inflation bug in Bitcoin" and I can just show them:
<img alt="image" src="https://github.com/bitcoin/bitcoin/assets/1841944/27aaed6f-7223-49a2-b40b-a5a6e71be434">

It looks more reassuring if it's organized as such.
What do you think?
⚠️ theStack opened an issue: "depends: `touch` command for creating determinstic archive timestamps fails on OpenBSD"
(https://github.com/bitcoin/bitcoin/issues/29447)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

In the course of the "Caching..." step of a depends build, the `touch` command for creating determinstic package archive timestamps fails on OpenBSD. The reason is that the used option `-h` option (introduced in PR #21995, commit 6ebe57622cb70df529879b15f291166177f2827c) doesn't exist in OpenBSD's version of touch (see https://man.openbsd.org/touch.1). E.g. for zeromq:
```
[ ... ]
Postp
...
💬 theStack commented on issue "depends: `touch` command for creating determinstic archive timestamps fails on OpenBSD":
(https://github.com/bitcoin/bitcoin/issues/29447#issuecomment-1951391552)
ping @hebasto
(Haven't investigated deeper what `-h` exactly does, but as far as I understand, the build still succeeds and there is no big harm in the failing `touch` command other than that the timestamps are not determinstic. Considering that OpenBSD is rather a niche OS, I think it's only worth fixing if there's an easy way to do so.)
🤔 fjahr reviewed a pull request: "doc: explain what the wallet password does"
(https://github.com/bitcoin/bitcoin/pull/28974#pullrequestreview-1887288612)
I would integrate this into the managing wallets doc rather than creating a new file. I also agree with @maflcko that putting this information into the users' path makes sense but I think this can still be added in the docs. It may not be enough to close the issue or maybe there should be a follow-up issue giving more detail.
💬 fjahr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1493809517)
I think this part can be shortened a lot.
💬 fjahr commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1493809487)
I don't think this is very relevant, even though it was suggested in the original issue.
💬 fjahr commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1951396971)
utACK fa1d4f07c3

There is still a reference to `error` in the comments in `logging.h`: https://github.com/fjahr/bitcoin/commit/6457d77e39a8c07fdef798f3bf115fcbb30545c4 But removing it can be kept for a follow-up.
🤔 fjahr reviewed a pull request: "RPC: access RPC arguments by name"
(https://github.com/bitcoin/bitcoin/pull/29277#pullrequestreview-1887298400)
Concept ACK, certainly makes the RPC code more readable.

@stickies-v have you considered preparing a map once when an RPC is called rather than doing the search for the index every time an arg is accessed?
💬 fjahr commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1493816010)
Indentation seems wrong?
💬 sdaftuar commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1493837935)
Should we also exclude the case where the sibling transaction has descendants of its own, which can happen as the result of a reorg? My concern is that our RBF rules today don't enforce incentive compatibility in such a situation, so it's not clear that we should support this case for now (post-cluster-mempool, this would be fine).
🤔 fjahr reviewed a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1887366958)
tACK d82eafb173d6bfa98a59e86a845013cc8528b65d
💬 fjahr commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#discussion_r1493880488)
nit: since we wait for the disconnect anyway it seems that passing a different `p2p_idx` each time isn't necessary, so the test could be simplified a tiny bit