💬 dimitaracev commented on pull request "wallet: move lock to the top of ReleaseWallet":
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1872344129)
Closed in favor of https://github.com/bitcoin/bitcoin/pull/29155 due to the branch name being misleading.
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1872344129)
Closed in favor of https://github.com/bitcoin/bitcoin/pull/29155 due to the branch name being misleading.
💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1438415389)
> The `g_printed_dir` guard is only needed because the user may be running more than one test case
This would break the PR goal in the current implementation. Only the last executed test datadir would be kept, the rest will be deleted.
I think we should include the test name in the path and create a datadir directory for each executed tests. Could pull most of what I did before: 53eb4980d561491798d0c8b0ee383c83ba193432.
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1438415389)
> The `g_printed_dir` guard is only needed because the user may be running more than one test case
This would break the PR goal in the current implementation. Only the last executed test datadir would be kept, the rest will be deleted.
I think we should include the test name in the path and create a datadir directory for each executed tests. Could pull most of what I did before: 53eb4980d561491798d0c8b0ee383c83ba193432.
📝 mjdietzx opened a pull request: "tests: add functional test for miniscript decaying multisig"
(https://github.com/bitcoin/bitcoin/pull/29156)
This is very closely based on [test/functional/wallet_multisig_descriptor_psbt.py](/test/functional/wallet_multisig_descriptor_psbt.py) both in code and concept. It should serve as some integration testing for Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4 and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the real world aligning this to halvenings would be nice).
<!--
*** Please remove the following help text before submitting:
...
(https://github.com/bitcoin/bitcoin/pull/29156)
This is very closely based on [test/functional/wallet_multisig_descriptor_psbt.py](/test/functional/wallet_multisig_descriptor_psbt.py) both in code and concept. It should serve as some integration testing for Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4 and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the real world aligning this to halvenings would be nice).
<!--
*** Please remove the following help text before submitting:
...
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1872346963)
In a [simulation](https://delvingbitcoin.org/t/gutterguard-and-coingrinder-simulation-results/279) comparing the `master` branch with the `CoinGrinder` branch playing through a scenario that performs about 5 000 payments with feerates sampled from 2023, ___CoinGrinder reduces the total transaction fees by 9.5%___.
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1872346963)
In a [simulation](https://delvingbitcoin.org/t/gutterguard-and-coingrinder-simulation-results/279) comparing the `master` branch with the `CoinGrinder` branch playing through a scenario that performs about 5 000 payments with feerates sampled from 2023, ___CoinGrinder reduces the total transaction fees by 9.5%___.
📝 LarryRuane converted_to_draft a pull request: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564)
This backward-compatible change would help with code review, testing, and debugging. When `test_bitcoin` runs, it creates a working or data directory within `/tmp/test_common_Bitcoin\ Core/`, named as a long random (hex) string.
This small patch does three things:
- If the (new) argument `-testdatadir=<datadir>` is given, use `<datadir>/test_temp` as the working directory
- When the test starts, remove `<datadir>/test_temp` if it exists from an earlier run (currently, it's presumed not to
...
(https://github.com/bitcoin/bitcoin/pull/26564)
This backward-compatible change would help with code review, testing, and debugging. When `test_bitcoin` runs, it creates a working or data directory within `/tmp/test_common_Bitcoin\ Core/`, named as a long random (hex) string.
This small patch does three things:
- If the (new) argument `-testdatadir=<datadir>` is given, use `<datadir>/test_temp` as the working directory
- When the test starts, remove `<datadir>/test_temp` if it exists from an earlier run (currently, it's presumed not to
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438429769)
I'd avoid the `std::vector::at` everywhere and use `std::vector::operator[]` instead, as `at` involves conditional branches to detect out-of-bounds access (well-predicted ones, but for a tight loop like this I do expect it to still be impactful). It's generally better to crash than to have UB due to out-of-bounds access, but inside performance-critical algorithms I think it's worth the extra review effort to make sure there are no out-of-bounds accesses.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438429769)
I'd avoid the `std::vector::at` everywhere and use `std::vector::operator[]` instead, as `at` involves conditional branches to detect out-of-bounds access (well-predicted ones, but for a tight loop like this I do expect it to still be impactful). It's generally better to crash than to have UB due to out-of-bounds access, but inside performance-critical algorithms I think it's worth the extra review effort to make sure there are no out-of-bounds accesses.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438411976)
It took me a while to figure out what the intent of each operation is. It may be worthwhile to document that here too. My belief is:
* ADD: make the next undecided transaction selected (ignoring equal-value transactions if the last one was unselected).
* SHIFT: the last selected transaction is bad, skip any configurations that select it, and ADD something after that.
* CUT: the last selected transaction is bad even when unselected, skip any configurations that select or unselect it, and ADD s
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438411976)
It took me a while to figure out what the intent of each operation is. It may be worthwhile to document that here too. My belief is:
* ADD: make the next undecided transaction selected (ignoring equal-value transactions if the last one was unselected).
* SHIFT: the last selected transaction is bad, skip any configurations that select it, and ADD something after that.
* CUT: the last selected transaction is bad even when unselected, skip any configurations that select or unselect it, and ADD s
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438411775)
Perhaps this explanation of the algorithm belongs more inside the body of the function than at the top, as it's explaining the implementation and not the interface?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438411775)
Perhaps this explanation of the algorithm belongs more inside the body of the function than at the top, as it's explaining the implementation and not the interface?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438428556)
Given that any of the {add, shift, cut} states ends with executing an add, and the end of the add operation (the block of code on this line and below) determines what the next iteration will be, I think it would be more natural to move this block to the beginning of the while loop, before the switch case. So then it becomes a processing loop that in every iteration decides what it will be doing (cut, shift, or add), without state carried between iterations? This doesn't work for "done", but that
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438428556)
Given that any of the {add, shift, cut} states ends with executing an add, and the end of the add operation (the block of code on this line and below) determines what the next iteration will be, I think it would be more natural to move this block to the beginning of the while loop, before the switch case. So then it becomes a processing loop that in every iteration decides what it will be doing (cut, shift, or add), without state carried between iterations? This doesn't work for "done", but that
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438435912)
One complication with the "move decision for add/shift/cut to beginning of loop" approach is that this skipping doesn't work anymore as-is. I'd suggest turning it into a while loop that increments `utxo_pool_index` until the skip condition is no longer satisfied.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438435912)
One complication with the "move decision for add/shift/cut to beginning of loop" approach is that this skipping doesn't work anymore as-is. I'd suggest turning it into a while loop that increments `utxo_pool_index` until the skip condition is no longer satisfied.
📝 SatoshiCopyright opened a pull request: "Licence MIT Original"
(https://github.com/bitcoin/bitcoin/pull/29157)
That must be the correct MIT license. Because it was taken from Satoshi Nakamoto (satoshinakam0t0@duck.com) original project Bitcoin-Blockchain.
<!--
*** 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 ho
...
(https://github.com/bitcoin/bitcoin/pull/29157)
That must be the correct MIT license. Because it was taken from Satoshi Nakamoto (satoshinakam0t0@duck.com) original project Bitcoin-Blockchain.
<!--
*** 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 ho
...
💬 SatoshiCopyright commented on pull request "Licence MIT Original":
(https://github.com/bitcoin/bitcoin/pull/29157#issuecomment-1872446772)
That must be the correct MIT license. Because it was taken from Satoshi Nakamoto original project Bitcoin.
(https://github.com/bitcoin/bitcoin/pull/29157#issuecomment-1872446772)
That must be the correct MIT license. Because it was taken from Satoshi Nakamoto original project Bitcoin.
💬 SatoshiCopyright commented on pull request "Licence MIT Original":
(https://github.com/bitcoin/bitcoin/pull/29157#issuecomment-1872448116)
**@SatoshiNT0 _is owner of Bitcoin._**
**### **
(https://github.com/bitcoin/bitcoin/pull/29157#issuecomment-1872448116)
**@SatoshiNT0 _is owner of Bitcoin._**
**### **
✅ achow101 closed a pull request: "Licence MIT Original"
(https://github.com/bitcoin/bitcoin/pull/29157)
(https://github.com/bitcoin/bitcoin/pull/29157)
📝 achow101 locked a pull request: "Licence MIT Original"
(https://github.com/bitcoin/bitcoin/pull/29157)
That must be the correct MIT license. Because it was taken from Satoshi Nakamoto (satoshinakam0t0@duck.com) original project Bitcoin-Blockchain.
<!--
*** 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 ho
...
(https://github.com/bitcoin/bitcoin/pull/29157)
That must be the correct MIT license. Because it was taken from Satoshi Nakamoto (satoshinakam0t0@duck.com) original project Bitcoin-Blockchain.
<!--
*** 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 ho
...
📝 darosior opened a pull request: "PoC: fuzz chainstate and block managers"
(https://github.com/bitcoin/bitcoin/pull/29158)
We don't have a fuzzing harness for our main consensus engine [0]. This PR introduces two new targets which respectively fuzz the `BlockManager` and `ChainstateManager` (process headers, blocks, reorgs and assert some invariants in doing so).
There is two main obstacles to achieving this: PoW and io. The blocks and chainstate databases can be stored in memory but blocks still need a valid proof of work and to be stored on disk. Niklas solved the first issue in #28043 by simply introducing a g
...
(https://github.com/bitcoin/bitcoin/pull/29158)
We don't have a fuzzing harness for our main consensus engine [0]. This PR introduces two new targets which respectively fuzz the `BlockManager` and `ChainstateManager` (process headers, blocks, reorgs and assert some invariants in doing so).
There is two main obstacles to achieving this: PoW and io. The blocks and chainstate databases can be stored in memory but blocks still need a valid proof of work and to be stored on disk. Niklas solved the first issue in #28043 by simply introducing a g
...
📝 bitnet-io opened a pull request: "Update net.h bigger TCP socket using larger buffer"
(https://github.com/bitcoin/bitcoin/pull/29159)
completely sync a 2gb prune in 8-10 hours instead of 3 days see src/net_processing.cpp also
prototype https://github.com/c4pt000/bitcoin 2 years ago testing
(https://github.com/bitcoin/bitcoin/pull/29159)
completely sync a 2gb prune in 8-10 hours instead of 3 days see src/net_processing.cpp also
prototype https://github.com/c4pt000/bitcoin 2 years ago testing
✅ maflcko closed a pull request: "Update net.h bigger TCP socket using larger buffer"
(https://github.com/bitcoin/bitcoin/pull/29159)
(https://github.com/bitcoin/bitcoin/pull/29159)
💬 furszy commented on pull request "wallet: move lock at the top of ReleaseWallet":
(https://github.com/bitcoin/bitcoin/pull/29155#discussion_r1438591791)
If this would be the issue presented in #29073, the test error logs would be showing a node crash. It wouldn't reach the `got_loading_error` check.
(https://github.com/bitcoin/bitcoin/pull/29155#discussion_r1438591791)
If this would be the issue presented in #29073, the test error logs would be showing a node crash. It wouldn't reach the `got_loading_error` check.
🤔 ismaelsadeeq reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-1779525637)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-1779525637)
Concept ACK