👍 TheCharlatan approved a pull request: "test: [refactor] Pass TestOpts"
(https://github.com/bitcoin/bitcoin/pull/30407#pullrequestreview-2169975415)
Nice, ACK fa690c8e532672f7ab53be6f7a0bb3070858745e
(https://github.com/bitcoin/bitcoin/pull/30407#pullrequestreview-2169975415)
Nice, ACK fa690c8e532672f7ab53be6f7a0bb3070858745e
📝 TheCharlatan opened a pull request: "kernel: De-globalize static validation variables"
(https://github.com/bitcoin/bitcoin/pull/30425)
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.
(https://github.com/bitcoin/bitcoin/pull/30425)
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.
💬 achow101 commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + test":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2221235072)
ACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2221235072)
ACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c
💬 achow101 commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2221253695)
ACK 8789dc8f315a9d9ad7142d831bc9412f780248e7
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2221253695)
ACK 8789dc8f315a9d9ad7142d831bc9412f780248e7
🚀 achow101 merged a pull request: "Assumeutxo: bugfix on loadtxoutset with a divergent chain + test"
(https://github.com/bitcoin/bitcoin/pull/29996)
(https://github.com/bitcoin/bitcoin/pull/29996)
🚀 achow101 merged a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668)
(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
(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?
(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.
(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).
(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.
(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:
(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()
```
(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
...
(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
...
(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
(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
(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
(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)
(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
...
(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
...