💬 pinheadmz commented on issue "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit"":
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1675072589)
> Will work on the test case and push it. Good eye reporting this @pinheadmz 👌🏼.
props to @Vasu-08 whose work on descriptors in bcoin brought this to my attention ;-)
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1675072589)
> Will work on the test case and push it. Good eye reporting this @pinheadmz 👌🏼.
props to @Vasu-08 whose work on descriptors in bcoin brought this to my attention ;-)
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291575150)
Yes, we are doing it manually, because we also compare it against what the mempool thinks it is.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291575150)
Yes, we are doing it manually, because we also compare it against what the mempool thinks it is.
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291575841)
I added an `assert(fee);` right before.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291575841)
I added an `assert(fee);` right before.
📝 Brotcrunsher opened a pull request: "Unified mixture of unnamed namespace and static."
(https://github.com/bitcoin/bitcoin/pull/28259)
Previously we had an unnamed namespace with two functions, one static the other not static. When briefly reading the code this might be slightly misleading and could make the reader think that not both functions are translation unit local, even though they are.
(https://github.com/bitcoin/bitcoin/pull/28259)
Previously we had an unnamed namespace with two functions, one static the other not static. When briefly reading the code this might be slightly misleading and could make the reader think that not both functions are translation unit local, even though they are.
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1675183615)
Sorry, I still need to squash this, but this should be addressing all the open comments
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1675183615)
Sorry, I still need to squash this, but this should be addressing all the open comments
⚠️ Brotcrunsher opened an issue: "getJsonToken assumes underlying string is null terminated"
(https://github.com/bitcoin/bitcoin/issues/28260)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When a function requires a `const char *end`, one might assume that the string doesn't need to be null terminated. `getJsonToken` requires this parameter:
```
enum jtokentype getJsonToken(std::string& tokenVal, unsigned int& consumed,
const char *raw, const char *end)
```
However, the function does assume that the underlying memory block is null-terminate
...
(https://github.com/bitcoin/bitcoin/issues/28260)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When a function requires a `const char *end`, one might assume that the string doesn't need to be null terminated. `getJsonToken` requires this parameter:
```
enum jtokentype getJsonToken(std::string& tokenVal, unsigned int& consumed,
const char *raw, const char *end)
```
However, the function does assume that the underlying memory block is null-terminate
...
💬 brunoerg commented on pull request "test: check backup from `migratewallet` can be successfully restored":
(https://github.com/bitcoin/bitcoin/pull/28257#issuecomment-1675289212)
Thanks, @furszy. Initially, I was thinking about identifying all backup files into the node's directory, run `migratewallet` with them and check the result. However, I agree it's better to go beyond by checking wallet's balance and other stuff prior and post migration.
Force-pushed changing the approach. I also moved it to `test_basic()`.
(https://github.com/bitcoin/bitcoin/pull/28257#issuecomment-1675289212)
Thanks, @furszy. Initially, I was thinking about identifying all backup files into the node's directory, run `migratewallet` with them and check the result. However, I agree it's better to go beyond by checking wallet's balance and other stuff prior and post migration.
Force-pushed changing the approach. I also moved it to `test_basic()`.
🤔 furszy reviewed a pull request: "wallet: Allow users to create a wallet that encrypts all database records"
(https://github.com/bitcoin/bitcoin/pull/28142#pullrequestreview-1574384593)
> @furszy That would work. My experience though is that touching anything inside Core's directories can get me in trouble. I once renamed a wallet file and Qt broke. It would only work again if I went into some JSON file to rename the wallet there too..
Well, that is not the case for loading the wallet from an external directory. No need to touch anything on your system. Just run the RPC command.
That being said, if you can trigger any crash by manually changing something on your system,
...
(https://github.com/bitcoin/bitcoin/pull/28142#pullrequestreview-1574384593)
> @furszy That would work. My experience though is that touching anything inside Core's directories can get me in trouble. I once renamed a wallet file and Qt broke. It would only work again if I went into some JSON file to rename the wallet there too..
Well, that is not the case for loading the wallet from an external directory. No need to touch anything on your system. Just run the RPC command.
That being said, if you can trigger any crash by manually changing something on your system,
...
🤔 fjahr reviewed a pull request: "assumeutxo (2)"
(https://github.com/bitcoin/bitcoin/pull/27596#pullrequestreview-1477561344)
Leaving some initial comments and will do some manual testing next.
@jamesob there are several todo comments in the code that sound like you were planning to address them before merge. Are you still planning to address those? Otherwise, could you update the comments so that we know what your plan is for those changes?
(https://github.com/bitcoin/bitcoin/pull/27596#pullrequestreview-1477561344)
Leaving some initial comments and will do some manual testing next.
@jamesob there are several todo comments in the code that sound like you were planning to address them before merge. Are you still planning to address those? Otherwise, could you update the comments so that we know what your plan is for those changes?
💬 fjahr commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291649621)
I am not sure it makes sense to distribute the prune budget between all chainstates equally. Ideally we would sync the background chainstate block by block and throw them away instantly because if we are pruning the likelihood that we keep the bg blocks at the end of IBD is much lower. I'm not sure what the perfect distribution would be but I would shift it more towards the assumed chainstate.
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291649621)
I am not sure it makes sense to distribute the prune budget between all chainstates equally. Ideally we would sync the background chainstate block by block and throw them away instantly because if we are pruning the likelihood that we keep the bg blocks at the end of IBD is much lower. I'm not sure what the perfect distribution would be but I would shift it more towards the assumed chainstate.
💬 fjahr commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291426426)
Better to be precise and ask them to only recommend deleting `blocks`, `chainstate` and `indexes`. Or alternatively add a reminder that they should backup their wallet before deleting the whole datadir.
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291426426)
Better to be precise and ask them to only recommend deleting `blocks`, `chainstate` and `indexes`. Or alternatively add a reminder that they should backup their wallet before deleting the whole datadir.
💬 fjahr commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291620032)
Hmm, might be good to update ZMQ and wallet docs as a follow-up that blocks that are connected to the background chain will not be notified...
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291620032)
Hmm, might be good to update ZMQ and wallet docs as a follow-up that blocks that are connected to the background chain will not be notified...
💬 fjahr commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291431562)
Doesn't `std::find_if` work for this?
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291431562)
Doesn't `std::find_if` work for this?
💬 fjahr commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291670706)
A bit simpler approach for this RPC would be to return to the user that the headers are not synced yet immediately and tell them they should try again when the headers are fully synced. I would be interested if others favor this approach as well.
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291670706)
A bit simpler approach for this RPC would be to return to the user that the headers are not synced yet immediately and tell them they should try again when the headers are fully synced. I would be interested if others favor this approach as well.
💬 russeree commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#issuecomment-1675558474)
Tested ACK - Using ```checklevel of 4```, ```assume valid=0```, ```reindex=1``` I was able to sync to the chain tip. This doesn't guarantee future consensus conflicts but should prove that there are not conflicts validating the 'main' chain. Testnet also validated successfully.
Full Settings
```
server=1
dbcache=16384
par=16
rpcthreads=8
rpcuser=bitcoin
rpcpassword=bitcoin
checkblocks=1
reindex=1
assumevalid=0
checklevel=4
maxconnections=0
listen=0
blocksonly=1
port=23766
rpc
...
(https://github.com/bitcoin/bitcoin/pull/28244#issuecomment-1675558474)
Tested ACK - Using ```checklevel of 4```, ```assume valid=0```, ```reindex=1``` I was able to sync to the chain tip. This doesn't guarantee future consensus conflicts but should prove that there are not conflicts validating the 'main' chain. Testnet also validated successfully.
Full Settings
```
server=1
dbcache=16384
par=16
rpcthreads=8
rpcuser=bitcoin
rpcpassword=bitcoin
checkblocks=1
reindex=1
assumevalid=0
checklevel=4
maxconnections=0
listen=0
blocksonly=1
port=23766
rpc
...
💬 jonatack commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1291926369)
> Do we have any risk of TOR and I2P peers bouncing each other forever?
I added logging and have been testing this, and haven't seen that occur thanks to the eviction protection as mentioned by AJ.
However, the observation and testing did find not-yet connected addnode peers being connected to as full outbound peers by the new logic.
It might be good not to do that, in order to avoid using our limited outbound slots for addnode peers, and to ensure that the peers a user selects to be ad
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1291926369)
> Do we have any risk of TOR and I2P peers bouncing each other forever?
I added logging and have been testing this, and haven't seen that occur thanks to the eviction protection as mentioned by AJ.
However, the observation and testing did find not-yet connected addnode peers being connected to as full outbound peers by the new logic.
It might be good not to do that, in order to avoid using our limited outbound slots for addnode peers, and to ensure that the peers a user selects to be ad
...
👍 jonatack approved a pull request: "bitcoin-tidy: fix macOS build"
(https://github.com/bitcoin/bitcoin/pull/28258#pullrequestreview-1574720222)
ACK bb3263d3e3d9f9d4db86dde679f469e7278bf737 tested with arm64 macos 13.5, llvm 16.0.6 and cmake 3.27.2
Did not test with cmake earlier than 3.27.
Note to testers, see the `/contrib/devtools/bitcoin-tidy/README` on current master at 3654d84c6f5 for the updated example usage docs.
```bash
jon|(bb3263d3e3d...):~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ cmake --version
cmake version 3.27.2
CMake suite maintained
...
(https://github.com/bitcoin/bitcoin/pull/28258#pullrequestreview-1574720222)
ACK bb3263d3e3d9f9d4db86dde679f469e7278bf737 tested with arm64 macos 13.5, llvm 16.0.6 and cmake 3.27.2
Did not test with cmake earlier than 3.27.
Note to testers, see the `/contrib/devtools/bitcoin-tidy/README` on current master at 3654d84c6f5 for the updated example usage docs.
```bash
jon|(bb3263d3e3d...):~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ cmake --version
cmake version 3.27.2
CMake suite maintained
...
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1675665941)
Chain backup servicing as a safety net definitely is interesting for witness and chain of transactions state or scarce collectible, when the value of the coins locked or “digital items” is worth more than a range of block miners fees. At least in term of Bitcoin crypto-economics IMHO, the good equilibrium with full-nodes resources I don’t know.
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1675665941)
Chain backup servicing as a safety net definitely is interesting for witness and chain of transactions state or scarce collectible, when the value of the coins locked or “digital items” is worth more than a range of block miners fees. At least in term of Bitcoin crypto-economics IMHO, the good equilibrium with full-nodes resources I don’t know.
💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1675667304)
@fanquake If you can reference this PR and this mailing list thread: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-June/thread.html started by the author of this PR in #28130 as I think the conversation is capturing good technical insights in practical ways to improve data carriage and archiving in the Bitcoin ecosystem. I was thinking to do it in #28130 though you locked the PR before.
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1675667304)
@fanquake If you can reference this PR and this mailing list thread: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-June/thread.html started by the author of this PR in #28130 as I think the conversation is capturing good technical insights in practical ways to improve data carriage and archiving in the Bitcoin ecosystem. I was thinking to do it in #28130 though you locked the PR before.
💬 ariard commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1675711759)
More than 2 years ago, the liquidity griefing provoked by RBF opt-out of a counterparty contributing to a multi-party transaction (splicing / dual-funding) was brought on the mailing list in a post intitled [“On Mempool Funny Games against Multi-Party Funded Transactions” ](https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-May/003033.html) and it was the main motivation behind the introduction of `mempoolfullrbf` with https://github.com/bitcoin/bitcoin/pull/25353.
Since then the
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1675711759)
More than 2 years ago, the liquidity griefing provoked by RBF opt-out of a counterparty contributing to a multi-party transaction (splicing / dual-funding) was brought on the mailing list in a post intitled [“On Mempool Funny Games against Multi-Party Funded Transactions” ](https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-May/003033.html) and it was the main motivation behind the introduction of `mempoolfullrbf` with https://github.com/bitcoin/bitcoin/pull/25353.
Since then the
...