Bitcoin Core Github
44 subscribers
120K links
Download Telegram
🚀 achow101 merged a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668)
👍 ryanofsky approved a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2169988165)
Code review ACK 0456828a50b86fd9185b7428e9012b7c18e970f9. Left some suggestions, but they are not important
💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1672799422)
In commit "logging: Add DisableLogging()" (ce34ad42c7c19edf726a31551ec3780733147db9)

Could there be a comment here explaining why logging is disabled here? Is it needed because this bitcoin-chainstate is not calling StartLogging(), so the log buffer could fill up?
💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1672814012)
In commit "logging: Limit early logging buffer" (3d4b1f4af406276671b5d0115574dea6b4a64340)

It seems like a mistake to drop this as it is not being set otherwise.
💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1672831214)
In commit "logging: Apply formatting to early log messages" (0456828a50b86fd9185b7428e9012b7c18e970f9)

Maybe something to consider for a followup but some of the arguments to FormatLogStrInPlace and the two LogPrintStr functions could be string_views instead of strings (logging_function, source_file, threadname).
💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1672811803)
In commit "logging: Limit early logging buffer" (3d4b1f4af406276671b5d0115574dea6b4a64340)

Since the log limit discards old log prints instead of skipping new prints, I think it would be a little clearer if "skipped" was replaced with "discarded" throughout this commit.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672873185)
> I expect that I will have to review and ack your changes many more times before it's merged

I certainly hope that isn't the case :sweat_smile:
💬 brunoerg commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1673007264)
```suggestion
// Can have rawnode under tr()
```
💬 TheBlueMatt commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2221479047)
> Because not everyone needs to run these things but if they want to they can. Cramming everything into one software project is not sustainable.

I don't think you read what I wrote. I said "Obviously I'm not claiming all of these things should be in Bitcoin core, but there's some cost to them being outside". Which implies, obviously I agree with you that "cramming everything into one software project is not sustainable", but claiming that *the* reason people don't run these projects is becaus
...
🤔 furszy reviewed a pull request: "test: fix inconsistency in fundrawtransaction weight limits test"
(https://github.com/bitcoin/bitcoin/pull/30353#pullrequestreview-2170403279)
Nice writeup @ismaelsadeeq!. Thanks for the push, just understood why it happened.

The issue is in the inputs selection. They are manually selected from the parent tx without excluding the change output. This output is not from the wallet and helps to surpass the selection amount (otherwise yes, the test would be failing all the time). So, the CI failure arises when the test excludes the change output which only occurs when the change output is at the end of the tx outputs.. which probability
...
💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2221624441)
Rebased
💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2221634539)
Rebased
💬 achow101 commented on pull request "[doc] archive v26.2 release notes":
(https://github.com/bitcoin/bitcoin/pull/30414#issuecomment-2221647342)
ACK 3c61cf3986d40ce0eae794f8a9571247f8af99e4
🚀 achow101 merged a pull request: "[doc] archive v26.2 release notes"
(https://github.com/bitcoin/bitcoin/pull/30414)
🤔 furszy reviewed a pull request: "test: fix inconsistency in fundrawtransaction weight limits test"
(https://github.com/bitcoin/bitcoin/pull/30353#pullrequestreview-2170533629)
Updated per feedback.

The CI failure that originated this PR (https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1657628378) can be replicated with the following patch in master:
```diff
diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py
--- a/test/functional/wallet_fundrawtransaction.py (revision 9b480f7a25a737c9c4ebc33401e94d66c2da9ec3)
+++ b/test/functional/wallet_fundrawtransaction.py (date 1720652934739)
@@ -1322,7 +1322,7
...
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2221722356)
With this merged, I will take #29770 out of draft mode shortly and also address the left-over comments here. Thanks @stickies-v and @furszy !
👍 tdb3 approved a pull request: "test: Fix intermittent failure in p2p_v2_misbehaving.py"
(https://github.com/bitcoin/bitcoin/pull/30420#pullrequestreview-2170625050)
re ACK 53f43293875de086cd0bd424fd582bc232bec3a3
Repeated test in https://github.com/bitcoin/bitcoin/pull/30420#pullrequestreview-2168907582
Would also re-review if maflcko's suggestions were implemented.
👍 tdb3 approved a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2170662623)
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
💬 tdb3 commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1673251107)
Yup, seems like its the most likely use case for assumeutxo, so I could see how that could be quite verbose.
👍 tdb3 approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2170666813)
ACK e1018672672f39910655ab37080bf3213ca55a39