💬 pinheadmz commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1158925769)
GREAT
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1158925769)
GREAT
💬 mzumsande commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1498015461)
I think that the data race is not the root cause, but happens as a result of the absolute timeout https://github.com/bitcoin/bitcoin/blob/04595484d97100a50c146e5fc080319d9e0f5ca4/src/test/txindex_tests.cpp#L35
not being sufficient, because the test environment is too slow for whatever reason.
In this case, the `BOOST_REQUIRE` in https://github.com/bitcoin/bitcoin/blob/04595484d97100a50c146e5fc080319d9e0f5ca4/src/test/txindex_tests.cpp#L38 fails, and the index isn't terminated correctly (by wa
...
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1498015461)
I think that the data race is not the root cause, but happens as a result of the absolute timeout https://github.com/bitcoin/bitcoin/blob/04595484d97100a50c146e5fc080319d9e0f5ca4/src/test/txindex_tests.cpp#L35
not being sufficient, because the test environment is too slow for whatever reason.
In this case, the `BOOST_REQUIRE` in https://github.com/bitcoin/bitcoin/blob/04595484d97100a50c146e5fc080319d9e0f5ca4/src/test/txindex_tests.cpp#L38 fails, and the index isn't terminated correctly (by wa
...
📝 willcl-ark opened a pull request: "Deprecate and remove BIP35 mempool p2p message"
(https://github.com/bitcoin/bitcoin/pull/27426)
The [BIP35](https://github.com/bitcoin/bips/blob/master/bip-0035.mediawiki) `mempool` P2P message can be used to share the txids in a node's mempool (motivation in BIP35). However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.
It was originally introduced with a protocol version bump from 60001 to 60002 and identification of a node willing to provide this service was based on this new protocol version and the advertisment of `NODE_NETWORK`.
S
...
(https://github.com/bitcoin/bitcoin/pull/27426)
The [BIP35](https://github.com/bitcoin/bips/blob/master/bip-0035.mediawiki) `mempool` P2P message can be used to share the txids in a node's mempool (motivation in BIP35). However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.
It was originally introduced with a protocol version bump from 60001 to 60002 and identification of a node willing to provide this service was based on this new protocol version and the advertisment of `NODE_NETWORK`.
S
...
💬 mzumsande commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1498111803)
Hmm, the existing timeout of 10 seconds seems plenty for the sync process, which takes just a few milliseconds on my computer.
@fanquake would a higher timeout fix this in your environment? Are you doing some extreme parallelization or someting, such that the test is simply that slow?
Or is there possibly some other reason that prevents the Index sync from finishing on aarch64, so that bumping the timeout wouldn't help?
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1498111803)
Hmm, the existing timeout of 10 seconds seems plenty for the sync process, which takes just a few milliseconds on my computer.
@fanquake would a higher timeout fix this in your environment? Are you doing some extreme parallelization or someting, such that the test is simply that slow?
Or is there possibly some other reason that prevents the Index sync from finishing on aarch64, so that bumping the timeout wouldn't help?
💬 dergoegge commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1498115249)
Concept ACK
(I would like to see us do the same for BIP37 at some point)
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1498115249)
Concept ACK
(I would like to see us do the same for BIP37 at some point)
💬 willcl-ark commented on issue "RPC `getblock` resulted in 500 and ReadBlockFromDisk: OpenBlockFile failed for FlatFilePos(nFile=-1, nPos=0)":
(https://github.com/bitcoin/bitcoin/issues/20978#issuecomment-1498130708)
> But in that case it shouldn't be 500, more like 400 Bad Request - it's not the user that failed, it's the client that requested a non-existing block. But again, it's only a theory.
This looks to me like it will be fixed by https://github.com/bitcoin/bitcoin/pull/27101 if requesting using json 2.0 spec, where we will return status code `200` (and only an `error` field). @pinheadmz might be able to confirm?
(https://github.com/bitcoin/bitcoin/issues/20978#issuecomment-1498130708)
> But in that case it shouldn't be 500, more like 400 Bad Request - it's not the user that failed, it's the client that requested a non-existing block. But again, it's only a theory.
This looks to me like it will be fixed by https://github.com/bitcoin/bitcoin/pull/27101 if requesting using json 2.0 spec, where we will return status code `200` (and only an `error` field). @pinheadmz might be able to confirm?
💬 RandyMcMillan commented on pull request "ci: fix git dubious permissions error":
(https://github.com/bitcoin/bitcoin/pull/27423#issuecomment-1498134466)
ACK ed4a833
(https://github.com/bitcoin/bitcoin/pull/27423#issuecomment-1498134466)
ACK ed4a833
🤔 EthanHeilman reviewed a pull request: "Fixes compile errors in MSVC build #27332"
(https://github.com/bitcoin/bitcoin/pull/27335)
(https://github.com/bitcoin/bitcoin/pull/27335)
💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1159011140)
done
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1159011140)
done
💬 jonatack commented on pull request "test: create random and coins utils, add amount helper, dedupe add_coin":
(https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1498148650)
> setup_common can't use any of the random functions without creating a circular dependency
Addressed with #27425.
(https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1498148650)
> setup_common can't use any of the random functions without creating a circular dependency
Addressed with #27425.
💬 TheCharlatan commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1498153284)
RE https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1373289140
> Just an option to consider, but I think could be good to move GetConfigFile, GetConfigOptions, ArgsManager::ReadConfigStream, and ArgsManager::ReadConfigFiles functions to src/common/config.cpp instead of src/common/args.cpp, since in longer run, it would be nice if it would be nice if there were a src/common/settings.cpp file to parse the settings file, src/common/config.cpp to parse the config file, and src/comm
...
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1498153284)
RE https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1373289140
> Just an option to consider, but I think could be good to move GetConfigFile, GetConfigOptions, ArgsManager::ReadConfigStream, and ArgsManager::ReadConfigFiles functions to src/common/config.cpp instead of src/common/args.cpp, since in longer run, it would be nice if it would be nice if there were a src/common/settings.cpp file to parse the settings file, src/common/config.cpp to parse the config file, and src/comm
...
💬 TheCharlatan commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1498165963)
Updated c02e451882bb1d220b944af3b444d4b529ac351a -> de2546e672d0a2103228d777b35bfa6aab4881ee ([splitSystemArgs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_1) -> [splitSystemArgs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemArgs_1..splitSystemArgs_2)):
* Addressed @hebasto's [comment](https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1158131412) adding a missing copyright
...
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1498165963)
Updated c02e451882bb1d220b944af3b444d4b529ac351a -> de2546e672d0a2103228d777b35bfa6aab4881ee ([splitSystemArgs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_1) -> [splitSystemArgs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemArgs_1..splitSystemArgs_2)):
* Addressed @hebasto's [comment](https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1158131412) adding a missing copyright
...
🤔 mzumsande reviewed a pull request: "Deprecate and remove BIP35 mempool p2p message"
(https://github.com/bitcoin/bitcoin/pull/27426)
> or anybody still actively using this message to find out more about their use-cases.
how about a mailing list post to reach a wider audience?
(https://github.com/bitcoin/bitcoin/pull/27426)
> or anybody still actively using this message to find out more about their use-cases.
how about a mailing list post to reach a wider audience?
💬 willcl-ark commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1498202324)
@kylezs Could you program your test to use the `--fallbackfee` if it recieves the error in reponse, or as Antoine reponded to you there fill up your test mempool so that `estimatesmartfee` works as intended?
Accessing `--fallbackfee` from within the fee estimation rpc involves importing a lot of wallet code which woudl be preferable to avoid...
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1498202324)
@kylezs Could you program your test to use the `--fallbackfee` if it recieves the error in reponse, or as Antoine reponded to you there fill up your test mempool so that `estimatesmartfee` works as intended?
Accessing `--fallbackfee` from within the fee estimation rpc involves importing a lot of wallet code which woudl be preferable to avoid...
🤔 pablomartin4btc reviewed a pull request: "wallet, tests: Expand and test when the blank wallet flag should be un/set"
(https://github.com/bitcoin/bitcoin/pull/25634)
re-ACK https://github.com/bitcoin/bitcoin/commit/c5fb9a4dd97b47db0e52c497a757dff943b255b1
(https://github.com/bitcoin/bitcoin/pull/25634)
re-ACK https://github.com/bitcoin/bitcoin/commit/c5fb9a4dd97b47db0e52c497a757dff943b255b1
📝 dimitaracev opened a pull request: "validation: Replicate MinBIP9WarningHeight with MinBIP9WarningStartTime"
(https://github.com/bitcoin/bitcoin/pull/27427)
This PR addresses [comment](https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1497009569). Replaces `CChainParams::MinBIP9WarningHeight` with `CChainParams::MinBIP9WarningStartTime`. It is then used in `WarningBitsConditionChecker::BeginTime` to only check block headers that are newer.
This helps reduce the block headers that are being scanned for warnings.
cc @ajtowns
(https://github.com/bitcoin/bitcoin/pull/27427)
This PR addresses [comment](https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1497009569). Replaces `CChainParams::MinBIP9WarningHeight` with `CChainParams::MinBIP9WarningStartTime`. It is then used in `WarningBitsConditionChecker::BeginTime` to only check block headers that are newer.
This helps reduce the block headers that are being scanned for warnings.
cc @ajtowns
👋 jonatack's pull request is ready for review: "test: move remaining rand code from util/setup_common to util/random"
(https://github.com/bitcoin/bitcoin/pull/27425)
(https://github.com/bitcoin/bitcoin/pull/27425)
👍 hebasto approved a pull request: "Fixes compile errors in MSVC build #27332"
(https://github.com/bitcoin/bitcoin/pull/27335)
re-ACK 6a9a4d13b2b22632e0acd4f86f7bac238294ba42
(https://github.com/bitcoin/bitcoin/pull/27335)
re-ACK 6a9a4d13b2b22632e0acd4f86f7bac238294ba42
💬 josibake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1159666836)
confirmed this does fix the download selection filter
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1159666836)
confirmed this does fix the download selection filter
💬 MarcoFalke commented on pull request "ci: Run base install at most once":
(https://github.com/bitcoin/bitcoin/pull/27429#discussion_r1159667906)
Yes, `DANGER_RUN_CI_ON_HOST` is called that way to discourage people from trashing their system
(https://github.com/bitcoin/bitcoin/pull/27429#discussion_r1159667906)
Yes, `DANGER_RUN_CI_ON_HOST` is called that way to discourage people from trashing their system