💬 fanquake commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1876945410)
> It makes a minor difference of reporting the correct version number.
What do you mean by/where are we reporting the version number?
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1876945410)
> It makes a minor difference of reporting the correct version number.
What do you mean by/where are we reporting the version number?
💬 crediblebytes commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876950847)
You never change software requirements to fit the functionality. You either revisit the requirements to "Build the right software" or make the fix to "Build the software right". There was no bug fix but a lazy man's attempt to hide the bug at best. Honestly this person would be reprimanded for such negligence. Put the textual description back to what it was and provide a fix. Ya'll puttin development to shame.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876950847)
You never change software requirements to fit the functionality. You either revisit the requirements to "Build the right software" or make the fix to "Build the software right". There was no bug fix but a lazy man's attempt to hide the bug at best. Honestly this person would be reprimanded for such negligence. Put the textual description back to what it was and provide a fix. Ya'll puttin development to shame.
📝 hebasto opened a pull request: "build: Fix check whether `-latomic` needed"
(https://github.com/bitcoin/bitcoin/pull/29177)
Clang >=15 still might need linking against `libatomic`.
We use `std::atomic<std::chrono::seconds>::compare_exchange_strong` in `net_processing.cpp`.
Addresses the https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694.
(https://github.com/bitcoin/bitcoin/pull/29177)
Clang >=15 still might need linking against `libatomic`.
We use `std::atomic<std::chrono::seconds>::compare_exchange_strong` in `net_processing.cpp`.
Addresses the https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440293694.
💬 fanquake commented on issue "Broken `--enable-suppress-external-warning` for Apple Clang 15 on `x86_64`":
(https://github.com/bitcoin/bitcoin/issues/29174#issuecomment-1876969998)
> fails with error: unknown argument: '-internal-isystem/usr/local/include'
Can this be investigated further. The `-Xclang` and `-internal-isystem` options still exist in Clang. Is the issue here just the concatenation? Maybe that worked previously, but no-longer does? What happens if you change `-Xclang -internal-isystem/usr/local/include` to `-Xclang -internal-isystem /usr/local/include`?
(https://github.com/bitcoin/bitcoin/issues/29174#issuecomment-1876969998)
> fails with error: unknown argument: '-internal-isystem/usr/local/include'
Can this be investigated further. The `-Xclang` and `-internal-isystem` options still exist in Clang. Is the issue here just the concatenation? Maybe that worked previously, but no-longer does? What happens if you change `-Xclang -internal-isystem/usr/local/include` to `-Xclang -internal-isystem /usr/local/include`?
💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441658150)
Please see https://github.com/bitcoin/bitcoin/pull/29177.
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1441658150)
Please see https://github.com/bitcoin/bitcoin/pull/29177.
💬 petertodd commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876973577)
On Thu, Jan 04, 2024 at 01:06:19AM -0800, Bastien Teinturier wrote:
> > No, that is not how Lightning works. With anchor outputs, beyond the basic fee level, fees are paid by whichever side wants to pay them, potentially both parties at once.
>
> You're confusing fees paid directly by the commitment transaction (which are always paid by the channel initiator, regardless of whose commitment transaction it is) and fees paid by CPFP-ing the anchor output. Which doesn't make any sense to me since
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876973577)
On Thu, Jan 04, 2024 at 01:06:19AM -0800, Bastien Teinturier wrote:
> > No, that is not how Lightning works. With anchor outputs, beyond the basic fee level, fees are paid by whichever side wants to pay them, potentially both parties at once.
>
> You're confusing fees paid directly by the commitment transaction (which are always paid by the channel initiator, regardless of whose commitment transaction it is) and fees paid by CPFP-ing the anchor output. Which doesn't make any sense to me since
...
💬 stickies-v commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876980521)
NACK. Rough consensus on behaviour change can be achieved on the other pull, we shouldn't have incorrect documentation while that process plays out.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1876980521)
NACK. Rough consensus on behaviour change can be achieved on the other pull, we shouldn't have incorrect documentation while that process plays out.
💬 petertodd commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876988120)
On Thu, Jan 04, 2024 at 03:19:50AM -0800, Gloria Zhao wrote:
> >> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
>
> > BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
> ago, when Lightning didn't even exist, and RBF's intended purpose was just to
> let people fee bump their own on-chain transactions. It was a much simpler
> time, and even then it got a BIP.
>
> A BIP was not required for RBF to be considered or merged. PR #
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1876988120)
On Thu, Jan 04, 2024 at 03:19:50AM -0800, Gloria Zhao wrote:
> >> This is a mempool policy proposal; I see no precedent that such a change requires a BIP.
>
> > BIP-125 Replace-By-Fee comes to mind... which was written just over 8 years
> ago, when Lightning didn't even exist, and RBF's intended purpose was just to
> let people fee bump their own on-chain transactions. It was a much simpler
> time, and even then it got a BIP.
>
> A BIP was not required for RBF to be considered or merged. PR #
...
⚠️ fanquake opened an issue: "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc"
(https://github.com/bitcoin/bitcoin/issues/29178)
master @ 65c05db660b2ca1d0076b0d8573a6760b3228068.
Running `time FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh` on Rawhide aarch64.
```bash
Run coincontrol with args ['/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/coincontrol')]crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x52d000000406 for type 'uint64_t' (aka 'u
...
(https://github.com/bitcoin/bitcoin/issues/29178)
master @ 65c05db660b2ca1d0076b0d8573a6760b3228068.
Running `time FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh` on Rawhide aarch64.
```bash
Run coincontrol with args ['/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/coincontrol')]crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x52d000000406 for type 'uint64_t' (aka 'u
...
💬 cculianu commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877009300)
> > Concept ACK
> > The transactions targeted by this pull-req. are a very significant source of prohibitive cost for regular node operators. It is very unlikely that regular node operators will give up mitigating that source of operative cost. Regular policy rule for these transactions would encourage the economical resource usage of mempools - not harming any miners - neither changing any fee estimation already on the field.
>
> As long as miners continue to mine these transactions they wi
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877009300)
> > Concept ACK
> > The transactions targeted by this pull-req. are a very significant source of prohibitive cost for regular node operators. It is very unlikely that regular node operators will give up mitigating that source of operative cost. Regular policy rule for these transactions would encourage the economical resource usage of mempools - not harming any miners - neither changing any fee estimation already on the field.
>
> As long as miners continue to mine these transactions they wi
...
💬 maflcko commented on pull request "build: Fix check whether `-latomic` needed":
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033482)
Testing on a fresh install of Ubuntu 24.04 Noble: `export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 && cd bitcoin && git fetch origin --depth=1 f8ca1357c8205ceff732dcfb0d2bad79b40b611b && git checkout f8ca1357c8205ceff732dcfb0d2bad79b40b611b && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3 make automake cmake curl clang-17 llvm-17 g++-multi
...
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033482)
Testing on a fresh install of Ubuntu 24.04 Noble: `export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 && cd bitcoin && git fetch origin --depth=1 f8ca1357c8205ceff732dcfb0d2bad79b40b611b && git checkout f8ca1357c8205ceff732dcfb0d2bad79b40b611b && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3 make automake cmake curl clang-17 llvm-17 g++-multi
...
💬 maflcko commented on pull request "build: Fix check whether `-latomic` needed":
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033681)
lgtm ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
(https://github.com/bitcoin/bitcoin/pull/29177#issuecomment-1877033681)
lgtm ACK f8ca1357c8205ceff732dcfb0d2bad79b40b611b
💬 michaelfolkson commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877045032)
Concept NACK.
1) The high fee rate, consensus valid transactions that Ocean mines is Ocean's business but trying to prevent high fee rate, consensus valid transactions from finding their way to **all** miners is a losing game. Even if this PR was merged the transactions could be submitted to miners out of band.
2) Transactions are just bytes with constraints imposed on them by the consensus rules. There is an endless list of ways small chunks of data (ie bytes) can be embedded in consensu
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877045032)
Concept NACK.
1) The high fee rate, consensus valid transactions that Ocean mines is Ocean's business but trying to prevent high fee rate, consensus valid transactions from finding their way to **all** miners is a losing game. Even if this PR was merged the transactions could be submitted to miners out of band.
2) Transactions are just bytes with constraints imposed on them by the consensus rules. There is an endless list of ways small chunks of data (ie bytes) can be embedded in consensu
...
💬 ajtowns commented on pull request "doc: revert clarify -datacarriersize":
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1877052212)
> You never change software requirements to fit the functionality.
The help text and documentation is not generally a statement of requirements, rather it documents what the software does, whether that's optimal/desirable or not. The closest thing we have to requirements docs are the BIPs, however the senior editor there [considers node policies](https://github.com/bitcoin/bips/pull/1524#issuecomment-1869734387) like this as being out of scope there as well.
(https://github.com/bitcoin/bitcoin/pull/29173#issuecomment-1877052212)
> You never change software requirements to fit the functionality.
The help text and documentation is not generally a statement of requirements, rather it documents what the software does, whether that's optimal/desirable or not. The closest thing we have to requirements docs are the BIPs, however the senior editor there [considers node policies](https://github.com/bitcoin/bips/pull/1524#issuecomment-1869734387) like this as being out of scope there as well.
💬 sipa commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877091223)
> What do you mean by/where are we reporting the version number?
The commit title, "Update secp256k1 subtree to upstream release 0.4.1", makes it sound like it's updating to the exact 0.4.1 tag. I agree it's fine to update to master, and that we shouldn't need a policy of only using tagged releases, but it'd be better if the commit title said "Update secp256k1 subtree to current master", or "to commit after 0.4.1".
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1877091223)
> What do you mean by/where are we reporting the version number?
The commit title, "Update secp256k1 subtree to upstream release 0.4.1", makes it sound like it's updating to the exact 0.4.1 tag. I agree it's fine to update to master, and that we shouldn't need a policy of only using tagged releases, but it'd be better if the commit title said "Update secp256k1 subtree to current master", or "to commit after 0.4.1".
💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1441747639)
This will fail to compile without sqlite support. Need to scope the test under the `USE_SQLITE` compile flag.
```diff
diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp
--- a/src/wallet/test/db_tests.cpp (revision f1b5b259f6d4759e7fc514b82b4096f7669e8325)
+++ b/src/wallet/test/db_tests.cpp (date 1704374489489)
@@ -228,6 +228,7 @@
}
}
+#ifdef USE_SQLITE
BOOST_AUTO_TEST_CASE(concurrent_txn_dont_interfere)
{
std::string key = "key";
@@ -269,6 +270,
...
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1441747639)
This will fail to compile without sqlite support. Need to scope the test under the `USE_SQLITE` compile flag.
```diff
diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp
--- a/src/wallet/test/db_tests.cpp (revision f1b5b259f6d4759e7fc514b82b4096f7669e8325)
+++ b/src/wallet/test/db_tests.cpp (date 1704374489489)
@@ -228,6 +228,7 @@
}
}
+#ifdef USE_SQLITE
BOOST_AUTO_TEST_CASE(concurrent_txn_dont_interfere)
{
std::string key = "key";
@@ -269,6 +270,
...
💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441757749)
nit (feel free to ignore):
```diff
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index 7885eda747..3f2a55f310 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -54,11 +54,6 @@ void init_block_index()
FUZZ_TARGET(block_index, .init = init_block_index)
{
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
- auto block_index = kernel::BlockTreeDB(DBParams{
- .path = "", // Memory only.
-
...
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441757749)
nit (feel free to ignore):
```diff
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index 7885eda747..3f2a55f310 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -54,11 +54,6 @@ void init_block_index()
FUZZ_TARGET(block_index, .init = init_block_index)
{
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
- auto block_index = kernel::BlockTreeDB(DBParams{
- .path = "", // Memory only.
-
...
💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441762014)
nit (feel free to ignore):
```diff
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index 7885eda747..bc073e4930 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -93,8 +93,8 @@ FUZZ_TARGET(block_index, .init = init_block_index)
// We should be able to read every block file info we stored. Its value should correspond to
// what we stored above.
- CBlockFileInfo info;
for (const auto& [n, file_info]: files_
...
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1441762014)
nit (feel free to ignore):
```diff
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index 7885eda747..bc073e4930 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -93,8 +93,8 @@ FUZZ_TARGET(block_index, .init = init_block_index)
// We should be able to read every block file info we stored. Its value should correspond to
// what we stored above.
- CBlockFileInfo info;
for (const auto& [n, file_info]: files_
...
👍 brunoerg approved a pull request: "fuzz: a target for the block index database"
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-1804183397)
crACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b
left some nits
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-1804183397)
crACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b
left some nits
🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1804185742)
> I don't think this particular issue is a problem for us however. The fact that it deadlocks in this particular test case is probably a good thing since it means that BDB is handling locking internally. If there were two threads trying to read/write at the same time, BDB would force one thread to wait for the other, as this PR is doing for SQLite. That's the behavior that we want. It's only a deadlock (and test failure) since the test is trying to do the behavior of two threads within one threa
...
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1804185742)
> I don't think this particular issue is a problem for us however. The fact that it deadlocks in this particular test case is probably a good thing since it means that BDB is handling locking internally. If there were two threads trying to read/write at the same time, BDB would force one thread to wait for the other, as this PR is doing for SQLite. That's the behavior that we want. It's only a deadlock (and test failure) since the test is trying to do the behavior of two threads within one threa
...
💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1441785861)
noted! will update
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1441785861)
noted! will update