📝 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
💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425282679)
In 671de055c8b3ff48b16b93d8389b80fead6342ac
Commit title indicate " [doc] cpfp carveout is excluded in packages "
But the diff is not a only a doc change.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425282679)
In 671de055c8b3ff48b16b93d8389b80fead6342ac
Commit title indicate " [doc] cpfp carveout is excluded in packages "
But the diff is not a only a doc change.
💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425551039)
In 228a5ed16586ef93e7ec5c5da3ef4df0d5fc322f
```cpp
#include <alogorithm>
```
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425551039)
In 228a5ed16586ef93e7ec5c5da3ef4df0d5fc322f
```cpp
#include <alogorithm>
```